-
Notifications
You must be signed in to change notification settings - Fork 300
[FB] [PI-3478] Add resolve doc-strings #3842
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
[FB] [PI-3478] Add resolve doc-strings #3842
Conversation
| if self.lhs_cube.ndim >= self.rhs_cube.ndim: | ||
| self.map_rhs_to_lhs = True | ||
| else: | ||
| self.map_rhs_to_lhs = False |
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.
Decided to make a slight infrastructure change to support the documentation of Resolve attribute members.
This involves pretty much taking the contents of _init and relocating it to __init__ so that sphinx captures the doc-strings (#:) of Resolve attribute members (this must physically happen in __init__ and not indirectly via calling _init from __init__, otherwise sphinx won't document them), and ensuring any other necessary logic is placed here in __call__.
| self._metadata_prepare() | ||
|
|
||
| return self | ||
|
|
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.
Returning self supports method chaining after making a call to a Resolve instance, which I wanted to do in the doc-tests e.g., now supports such patterns like
resolve = Resolve()
resolve(c1, c2).map_rhs_to_lhswhich is pretty handy for certain workflows
| free_flip = len(tgt_free) > len(src_free) | ||
| free_flip = src_cube.ndim == tgt_cube.ndim and len(tgt_free) > len( | ||
| src_free | ||
| ) |
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 was merged to the feature-branch cube-arithmetic-post as part of #3805
| self.category_common = _CategoryItems( | ||
| items_dim=[], items_aux=[], items_scalar=[] | ||
| ) | ||
|
|
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.
Do this here, rather in the now defunct _init.
And we don't want to do this within __init__, as we want it to happen per __call__
|
|
||
| @property | ||
| def _src_cube(self): | ||
| assert self.map_rhs_to_lhs is not 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.
Defensive code to ensure the _init restructuring to __init__ et al, hasn't had any side-effects that we don't know about.
But in general, it's a healthy assert to adopt IMHO
| result = None | ||
| if self.mapping is not None: | ||
| result = self._src_cube.ndim == len(self.mapping) | ||
| return result |
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.
Adds a bit more rigour to deal with the case where the user attempt to determine mapped, but no operands have actually been provisioned to Resolve
| data = np.zeros(cube1.shape) | ||
| data1 = data * 10 | ||
| data2 = data * 20 | ||
| data3 = data * 30 |
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.
Note that, the testsetup instances created here for this doc-string is available for all other doc-strings, thanks to the magic of sphinx.
Hence, there is no need to replicate the testsetup directive per doc-string test.
| STASH: m01s03i236 | ||
| source: Data from Met Office Unified Model 6.05 | ||
| Cell methods: | ||
| mean: time (6 hour) |
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.
The doc-test equivalent of a picture to 1000 words... there is no need to describe what's happening to a detailed level... just show it. Works for me as a philosophy 😉
|
thanks for the hangout @bjlittle to walk thru this. LGTM. |
|
@tkknight Awesome, thanks 👍 |
* Add resolve doc-strings * reorder some doctest statements
This PR adds doc-strings and doc-tests primarily for
iris.common.resolve.ResolveNote that, full test coverage for
Resolveis to follow as part of the roadmap for cube arithmetic - as agreed offline with other core devs