Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(ready for review) Draft docstring template #690

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

tennlee
Copy link
Collaborator

@tennlee tennlee commented Sep 13, 2024

Here is a draft spec that we could improve and consider including.
I have merged it into branch 228 so it can be seen at : https://scores.readthedocs.io/en/228-documentation-testing-branch/docstring_template.html

@tennlee

This comment was marked as resolved.

@tennlee
Copy link
Collaborator Author

tennlee commented Sep 13, 2024

Partly motivated by #619

@Steph-Chong

This comment was marked as resolved.

docs/docstring_spec.md Outdated Show resolved Hide resolved
docs/docstring_spec.md Outdated Show resolved Hide resolved
docs/docstring_spec.md Outdated Show resolved Hide resolved
docs/docstring_spec.md Outdated Show resolved Hide resolved
docs/docstring_spec.md Outdated Show resolved Hide resolved
@Steph-Chong

This comment was marked as resolved.

@Steph-Chong

This comment was marked as resolved.

@Steph-Chong

This comment was marked as resolved.

@tennlee tennlee changed the title (under development) Draft docstring spec (under development) Draft docstring template Sep 16, 2024
docs/docstring_spec.md Outdated Show resolved Hide resolved
docs/docstring_spec.md Outdated Show resolved Hide resolved
@tennlee tennlee changed the title (under development) Draft docstring template (ready for review) Draft docstring template Nov 3, 2024
@tennlee
Copy link
Collaborator Author

tennlee commented Nov 3, 2024

@nicholasloveday , I have gone through this in detail with @Steph-Chong . We may take another look after a bit of a break, but it's ready for you to take a look at and let us know what you think. If all seems well to you, I'm happy to merge it and then do any further work in subsequent issues, so long as it's good enough. Once it's in, we can also add a link to it in the PR checklist/template/thing.

I have merged it into https://scores.readthedocs.io/en/228-documentation-testing-branch/docstring_template.html along with other changes.

Copy link
Collaborator

@nicholasloveday nicholasloveday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this. A few minor suggestions

If there is general material like a wikipedia link, other docs site etc, add it here also
See also:
If there are closely related functions, add e.g. :py:func:`scores.continuous.rmse` with a note
Examples:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that it would be good if we can add examples in most functions.
Here is an issue that I created #692

>>> from scores.probability import interval_tw_crps_for_ensemble
>>> fcst = xr.DataArray(np.random.uniform(-40, 10, size=(10, 10)), dims=['time', 'ensemble'])
>>> obs = xr.DataArray(np.random.uniform(-40, 10, size=10), dims=['time'])

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
>>> tail_tw_crps_for_ensemble(fcst, obs, 'ensemble', 0.5, tail='upper')

Just adding a line for where the function is called for completeness


The layout below will render properly in Read the Docs. Please adjust the example text and MathJax as required.

Note: backticks render differently in docstrings to elsewhere. Should you wish to include text in backticks, please use double backticks - e.g. ``scores.continuous.mean_error``. (If you only use one set of backticks, the text will instead be rendered as italics in Read the Docs).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the scores.continuous.mean_error example, when writing the name of a function it is better to use ":py:func:`"

Consider updating this to suggest writing function names with ":py:func:" and non-functions that you want to render as a block (or whatever it's called) with double backtracks (e.g., reduce_dims="all"). I hope that makes sense

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that you use ":py:func:" further down in the example. Consider just updating this example to something like reduce_dims="all"

'''
A short one or two line description of what the function does.

Additional information (e.g. a paragraph or two) may be included here following the short description.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Longer lines aren't wrapping on 228. I suggest limiting the line length so that it renders correctly


.. math::
\\text{mean error} =\\frac{1}{N}\\sum_{i=1}^{N}(x_i - y_i)
\\text{where } x = \\text{the forecast, and } y = \\text{the observation}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be split into two sections, otherwise the second line becomes part of the first equation. I suggest just using :math:` around the x and y

@tennlee tennlee changed the title (ready for review) Draft docstring template (not quite ready for 1.3) Draft docstring template Nov 14, 2024
@tennlee tennlee changed the title (not quite ready for 1.3) Draft docstring template (ready for review) Draft docstring template Nov 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants