-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Migration of datatree/ops.py -> datatree_ops.py #8976
Migration of datatree/ops.py -> datatree_ops.py #8976
Conversation
I considered wedging this into core/ops.py, but the datatree/ops.py stuff is kind of spread into core/ops.py and generated_aggregations.py.
These are the only docstring that have a leading space and that was causing problems injecting the map_over_subtree information in the Datatree doc strings.
This works on most of the docstrings. The DatasetOpsMixin functions (round, argsorg, conj and conjugate) have different format and this gets inserted after the name (which is non standard in most docs) but before the description.
just for clarity.
Just syntactic sugar to make that work
Ok. The test build for docs is available to examine here https://xray--8977.org.readthedocs.build/en/8977/generated/xarray.DataTree.expand_dims.html#xarray.DataTree.expand_dims |
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.
Makes sense and the compiled documentation looks good in the RTD build. I think I only had one question really. Nice work!
@@ -824,3 +826,79 @@ def test_tree(self, create_test_datatree): | |||
expected = create_test_datatree(modify=lambda ds: np.sin(ds)) | |||
result_tree = np.sin(dt) | |||
assert_equal(result_tree, expected) | |||
|
|||
|
|||
class TestDocInsertion: |
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.
Nice tests.
don't, just have the addendum appeneded after. None values are returned. | ||
|
||
""" | ||
if docstring is None: |
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.
Nit: I think docstring
is guarded against being None
from the call to this function being wrapped in if orig_method_docstring is not None
. It doesn't look like this function is called elsewhere, but I might be missing something.
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.
It is, and I couldn't decide if I should take it out above or leave it here, so it's belts and braces.
xarray/core/datatree_ops.py
Outdated
setattr(target_cls_dict[method_name], "__doc__", new_method_docstring) | ||
|
||
|
||
def insert_doc_addendum(docstring, addendum): |
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.
Nit: Is it overkill to suggest type hints for this function?
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.
probably not
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.
|
||
This method was copied from xarray.Dataset, but has been altered to call the | ||
method on the Datasets stored in every node of the subtree. See the | ||
`map_over_subtree` function for more details.""" |
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.
So, when this message is just appended on the end of a single paragraph/line docstring, it isn't a note? I don't have strong opinions, but just highlighting the difference and asking if it should still be a note.
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.
Yes, I already don't know if I did this on purpose or not. All of the single
line docstrings from from DatasetOpsMixin dunder methods,
e.g.
DatasetOpsMixin.__ifloordiv__ 'Same as a //= b.'
DatasetOpsMixin.__rfloordiv__ 'Same as a // b.'
DatasetOpsMixin.__rmod__ 'Same as a % b.'
etc.
I don't see in the RTD docs if those are not built, or if I just didn't insert
them. I can't find any of those on the Dataset objects either. I think I was
thinking you'd most likely see the doc in a repl, but who knows why that's what
I was thinking..
This looks great, thank you @flamingbear ! Happy to merge? |
I am. I might take on docs next. |
I considered wedging this into core/ops.py, but it didn't look like it fit there.
This is a basic lift and shift from datatree_/ops.py to core/datatree_ops.py
I did fix the document addendum injection and added a couple of tests.
whats-new.rst