Properties giving order of types in inputs to NDCube.crop and crop_by_values 2 #868
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Cadair
reviewed
Jun 30, 2025
Comment on lines
+1464
to
+1467
| Returns types and order of high-level coordinate objects that must be input to .crop() | ||
| Each point given to .crop() must be a tuple of scalar high-level coordinate objects | ||
| of these types, in this order. Note, however, that any of these can be replaced | ||
| with None if cropping along the axis/axes of that world type in not desired. |
There was a problem hiding this comment.
First line of the docstring should be <80 char. Then a blank line, then a paragraph. (Same below)
There was a problem hiding this comment.
Also put code snippets in double backticks.
Cadair
reviewed
Jun 30, 2025
| @property | ||
| def crop_by_values_input_units_order(self): | ||
| """ | ||
| Returns units of inputs to .crop_by_values() and their required order. |
There was a problem hiding this comment.
Properties don't return they are.
Suggested change
| Returns units of inputs to .crop_by_values() and their required order. | |
| Units of inputs to ``.crop_by_values()`` and their required order. |
Cadair
reviewed
Jun 30, 2025
| return tuple(v[0] for v in self.wcs.world_axis_object_classes.values()) | ||
|
|
||
| @property | ||
| def crop_by_values_input_units_order(self): |
There was a problem hiding this comment.
I'm much less convinced by the need for this property as we can just say "look at cube.wcs.world_axis_units" in the docstring of crop_by_values we don't need this proxy.
Cadair
reviewed
Jun 30, 2025
| @@ -1458,6 +1458,25 @@ def fill_masked(self, fill_value, uncertainty_fill_value=None, unmask=False, fil | |||
| self.mask = False | |||
| return None | |||
|
|
|||
| @property | |||
| def crop_input_types_order(self): | |||
There was a problem hiding this comment.
This really feels like it belongs on the WCS API (as world_classes_order) or something.
|
I have opened a PR to astropy suggesting this property in the WCS API: astropy/astropy#18408 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Inspired by discussions in #866
Re-creation of PR #867 due to messing up git history.