Docstrings#111
Conversation
henrikjacobsenfys
added
[scope] bug
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #111 +/- ##
==========================================
Coverage ? 98.13%
==========================================
Files ? 36
Lines ? 2302
Branches ? 380
==========================================
Hits ? 2259
Misses ? 23
Partials ? 20
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| area (Int or float): Area under the curve. | ||
| unit (str or sc.Unit): Unit of the parameters. | ||
| Defaults to "meV". | ||
| area (Int | float): Area under the curve. center (Int | float): |
There was a problem hiding this comment.
I think you forgot a line break before center.
|
|
||
| Raises: | ||
| TypeError: If the value is not a number. | ||
| ValueError: If the value is not positive. |
There was a problem hiding this comment.
I don't think you need the Raises information . . .
rozyczko
left a comment
rozyczko
left a comment
There was a problem hiding this comment.
Looks good with several minor issues raised
| I(x) = \frac{A}{\\sigma \\sqrt{2\\pi}} | ||
| \\exp\\left( | ||
| -\frac{1}{2} | ||
| \\left(\frac{x - x_0}{\\sigma}\right)^2 | ||
| \right) |
There was a problem hiding this comment.
Is this correct? You are mixing unescaped \frac with \\sigma, \\sqrt etc. The class level docstring uses r""" with single backslashes. Is the rendered document OK?
I think you should just use single backslashes throughout, given you have r"""
There was a problem hiding this comment.
You're right, it needed r""". If I forget the r""" then Ruff automatically adds \
| r"""Test Model of a Gaussian function. | ||
|
|
There was a problem hiding this comment.
Is this still a "Test" model? Neither Lorentzian nor DHO have "Test"
There was a problem hiding this comment.
Whoops that "test" was added to check that the documentation updated when I made changes - didn't realise I pushed that change
| @@ -19,17 +19,30 @@ class Voigt(CreateParametersMixin, ModelComponent): | |||
| center is not provided, it will be centered at 0 and fixed, which is | |||
There was a problem hiding this comment.
Maybe you should use r""" for consistency? Even if there's no Latex in the docstrings
There was a problem hiding this comment.
Yeah probably. Thanks!
| unique_name (str | None): Unique name of the component. if None, | ||
| a unique_name is automatically generated. |
| energy (number, list, np.ndarray, | scipp Variable). The energy | ||
| transfer. If number, assumed to be in meV unless energy_unit |
There was a problem hiding this comment.
Stray | before scipp Variable.
Please standardise on full stop vs. colon after arg description
You have Parameter). If number and |None): Unit for below.
Please standardise on whether you want to use commas for arguments or vertical bars.
| TODO: change to sc.Variable? | ||
|
|
There was a problem hiding this comment.
TODOs are source code comments, NOT docs!
There was a problem hiding this comment.
Or github issues.. removed it
|
looks good now! |
3 participants
Improve docstrings for all ModelComponents.
Also prevented the setters of the width of Gaussian, Lorentzian and Voigt to set negative widths