-
-
Notifications
You must be signed in to change notification settings - Fork 53
Support Subtract and Division of NDCube by NDData #880
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
base: main
Are you sure you want to change the base?
Conversation
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.
I've tried to wrap my head around what's happening here and I don't see anything obviously problematic (though I don't know much about NDData either). My one suggestion would be adding a test that constructs an NDData from a dask array (I think this is possible??) and then apply a these operations (at least add and mul) to that object and ensure that the laziness is preserved in these new codepaths.
…preserve dask laziness.
@wtbarnes: Do these tests satisfy your comment? |
0c2e688
to
1bcd97f
Compare
f90bf15
to
9f9805f
Compare
@Cadair it looks like |
Oh, of course it doesn’t. Well balls. I'm not really sure what the best option is in that case then. |
Dare a say it? |
Do it |
…to nddataArithmetic2
>>> import warnings | ||
>>> with warnings.catch_warnings(): | ||
... warnings.simplefilter("ignore") # Catching warnings not needed but keeps docs cleaner. |
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.
I'm not a fan, it makes the warnings surprising for people following along at home. What warnings are we squashing?
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.
If I remember right, a warning is raised because NDUncertainty.propagate
doesn't support power, and so the uncertainties get dropped.
What alternative do you suggest?
where addition, subtraction, multiplication and division are all enabled by the ``+``, ``-``, ``*``, and ``/`` operators, respectively. | ||
|
||
Note that `~ndcube.NDCube` attributes not supported by the constructor of the output type employed by `ndcube.NDCube.to_nddata` are dropped by the conversion. | ||
Therefore, since `ndcube.NDCube.to_nddata` converts to `~astropy.nddata.NDData` by default, there was no need in the above example to explicitly set `~ndcube.NDCube.extra_coords` and `~ndcube.NDCube.global_coords` to ``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.
Why would you anyway?
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.
To make sure all coordinates are dropped if you didn't know that NDData
didn't support extra_coords
and global_coords
.
Note that the output type of `ndcube.NDCube.to_nddata` can be controlled via the ``nddata_type`` kwarg. | ||
For example: | ||
|
||
>>> from astropy.nddata import NDDataRef | ||
>>> nddataref2 = cube2.to_nddata(wcs=None, nddata_type=NDDataRef) | ||
>>> print(type(nddataref2) is NDDataRef) | ||
True |
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 feels irrelevant to the narrative here and just documentation of to_nddata
, and therefore in the wrong place?
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.
Fair. Currently, there isn't a section anywhere on to_nddata
and this code snippet probably doesn't merit one? So it might as well live here unless/until there is one.
`ndcube.NDCube.to_nddata` is not limited to changing/removing the WCS. | ||
The value of any input supported by the ``nddata_type``'s constructor can be altered by setting a kwarg for that input, e.g.: | ||
|
||
.. code-block:: python | ||
>>> nddata_ones = cube2.to_nddata(data=np.ones(cube2.data.shape)) | ||
>>> nddata_ones.data | ||
array([[1., 1., 1.], | ||
[1., 1., 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.
Same as this section, feels like the wrong place for this example? It's not relevant to arithmetic operations?
Co-authored-by: Stuart Mumford <[email protected]>
To Do