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

Migration of datatree/ops.py -> datatree_ops.py #8976

Merged
merged 12 commits into from
May 2, 2024

Conversation

flamingbear
Copy link
Member

@flamingbear flamingbear commented Apr 26, 2024

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.

  • Contributes to migration step for miscellaneous modules in Track merging datatree into xarray Track merging datatree into xarray #8572
  • Tests added
  • User visible changes (including notable bug fixes) are documented in whats-new.rst

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 syntactic sugar to make that work
@flamingbear flamingbear marked this pull request as ready for review April 26, 2024 20:20
@flamingbear
Copy link
Member Author

flamingbear commented Apr 26, 2024

Here's a sample of a doc page generated with these updates. I will point to the RTD when that finishes.

Screenshot 2024-04-26 at 2 07 38 PM

@flamingbear
Copy link
Member Author

Copy link
Contributor

@owenlittlejohns owenlittlejohns left a 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:
Copy link
Contributor

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:
Copy link
Contributor

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.

Copy link
Member Author

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.

setattr(target_cls_dict[method_name], "__doc__", new_method_docstring)


def insert_doc_addendum(docstring, addendum):
Copy link
Contributor

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

probably not

Copy link
Member Author

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."""
Copy link
Contributor

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.

Copy link
Member Author

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..

@TomNicholas TomNicholas added the topic-DataTree Related to the implementation of a DataTree class label May 2, 2024
@TomNicholas
Copy link
Member

This looks great, thank you @flamingbear ! Happy to merge?

@flamingbear
Copy link
Member Author

Happy to merge?

I am. I might take on docs next.

@TomNicholas TomNicholas merged commit f5ae623 into pydata:main May 2, 2024
34 checks passed
andersy005 added a commit that referenced this pull request May 3, 2024
* origin/main:
  call `np.cross` with 3D vectors only (#8993)
  Mark `test_use_cftime_false_standard_calendar_in_range` as an expected failure (#8996)
  Migration of datatree/ops.py -> datatree_ops.py (#8976)
  avoid a couple of warnings in `polyfit` (#8939)
@flamingbear flamingbear deleted the mhs/DAS-2065/migrate-ops branch May 21, 2024 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-DataTree Related to the implementation of a DataTree class
Projects
Development

Successfully merging this pull request may close these issues.

3 participants