-
Notifications
You must be signed in to change notification settings - Fork 133
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
DOC: Numpy docstrings migration #1341
DOC: Numpy docstrings migration #1341
Conversation
When using `this tutorial <https://link1.com>`_ and `this tutorial <https://link2.com>`_ in two spots, theres a colision due to creating the "this tutorial" reference. Using a double underscore makes it an anonymous link resolving the issue
Only adding napolean for now to visualise docstrings. Will add numpydocs to validate later one the docstrings are in a better state.
Also remove invalid syntax in field.py
Worked on following modules: parcels.interaction parcels.interaction.neighborsearch parcels.compilation parcels.rng parcels.tools.statuscodes parcels.tools.converters parcels.tools.loggers parcels.plotting parcels.scripts.plottrajectoriesfile parcels.scripts.get_examples
Also convert codecompiler
Done with initial conversion of docstrings to numpy format. Now moving onto docstring validation. |
RTD handling docs means that old build path wasn't necessary. Updating to _build (scripts are still useful for local debugging)
Numpydoc validators have been included in the build process (basically linting through docstrings, looking for things like "is the argument in the docstring the same as the one in the function call", ensuring the docstring is rendered correctly). The full list of possible validation checks are outlined in the error codes. Some of the codes are incompatible with the codebase (either outright, or due to the state of many docstrings). Implementing all these codes is a very very low priority, but the small subset of codes at the moment are a good starting point (and additional codes can be included easily down the line if desired). |
Just confirming @erikvansebille , is this option checked in the |
Yep, that checkbox is on (see below), but the default branch is not set. Should I do that? |
Following on from this (10 year old) conversation, it doesn't look like builds are triggered from PRs originating from branches on forks. No matter though. My RTD build for this branch is available here. We'll review properly when merging
No need 😄 |
These methods do not take a `field` parameter.
add_constant_field does not take parameter "units"
Thanks for digging into this
That link seems to point to the RTD Issue? Can you update to the correct link? |
Oops, yes, just updated |
Used the regex PR ready for feedback/merge 😄 (or we can review it all at once when we merge |
+-----------------------------+-----------------------------+-----------------------------+ | ||
| | V[k,j+1,i+1] | | | ||
+-----------------------------+-----------------------------+-----------------------------+ | ||
|U[k,j+1,i] |W[k:k+2,j+1,i+1],T[k,j+1,i+1]|U[k,j+1,i+1] | | ||
+-----------------------------+-----------------------------+-----------------------------+ | ||
| | V[k,j,i+1] + | | ||
+-----------------------------+-----------------------------+-----------------------------+ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tables don't show as nice as they do in the old version? Compare https://veckoparcels.readthedocs.io/en/docstrings/#parcels.fieldset.FieldSet.from_c_grid_dataset to https://parcels.readthedocs.io/en/latest/#parcels.fieldset.FieldSet.from_c_grid_dataset
Great work, @VeckoTheGecko; this new doc-formatting is much more pleasing to the eye indeed. We're not nearly at the same level of bumpy, but at least we're moving in the right direction. ;-) A few outstanding general points, before we merge:
|
As of 398ba3b the version number is determined automatically using the
Can do. Should we dynamically determine the year for the copyright statement? i.e. update the copyright statement from: copyright = '2022, The OceanParcels Team' to copyright = f"{datetime.datetime.now().year}, The OceanParcels Team" Happy to fix this in a separate PR.
I don't think so. The purpose of |
OK is fine, let's then just go for it. The integration tests have skipped (not sure why), but I guess we can merge without these in this case, because only docstrings are affected in this PR? |
#1335 rearing its head again 😕 And yes, only docstrings/doc related changes. |
This PR focusses on the following features (in order of implementation):
rst
docstrings tonumpydoc
format (with the help of pyment)numpydoc
docstring validators to ensure docstring qualitynumpydoc
docstring validators as part of the Sphinx build processPR contributes to #1324, where you can read more on this featureset.
Draft PR for visibility (and to trigger RTD builds).
(given the size of these diffs, let me know if you want me to break into multiple PRs)