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

Returning scalars from NDData #1314

Closed
embray opened this issue Aug 1, 2013 · 14 comments
Closed

Returning scalars from NDData #1314

embray opened this issue Aug 1, 2013 · 14 comments

Comments

@embray
Copy link
Member

embray commented Aug 1, 2013

Currently if one wants to return a scalar value from an NDData object it's a bit of a mess:

In [1]: from astropy.nddata import NDData

In [2]: d = NDData([1, 2, 3])

In [3]: d[0]
Out[3]: <astropy.nddata.nddata.NDData at 0x2b33e90>

In [4]: d[0].data
Out[4]: array(1)

In [5]: d[0].data.item()
Out[5]: 1

Right now even a scalar is wrapped in the NDData class with metadata and everything. I realize there are issues with this involving masks, error propagation, units (for arrays with units a scalar can just be returned as a Quantity object). But I think that right now for simple cases like, "I just want an array with metadata attached" the current behavior is bad. It would be better to implement reasonable behavior for now than to leave the class in this somewhat unusable state in anticipation of "eventually" solving all conceivable problems with how the class should ideally work.

@Cadair
Copy link
Member

Cadair commented Aug 1, 2013

I would think that a quantity is the way to go, the only obvious metadata loss in the current implementation would be coordinate data. (?) Which would be avoided if slicing by coordinate is implemented. I think returning a quantity is the most sensible option.

@astrofrog
Copy link
Member

Potentially controversial (this has come up before) - now that Quantity inherits from np.ndarray, should NDData inherit and extend Quantity? It would behave more intuitively for people used to Quantity then.

@Cadair
Copy link
Member

Cadair commented Aug 1, 2013

I, from a SunPy point of view, would be adverse to NDData inheriting ndarray in any form, having just removed this situation from our code base!!

@astrofrog
Copy link
Member

Ok - I just wanted to make sure we considered that option, but it would be great to have your insight into why you decided to move away from this?

@embray
Copy link
Member Author

embray commented Aug 2, 2013

I think the thing that makes NDData complicated is that it's really a container for multiple arrays that need to stay associated with each other. Still, I don't like that we now have these two specialized array types that have very different purposes. Can a Quantity array be used as the science data in an NDData? Has this even been tested?

I think the NDData class really needs some loving now, as I want to be able to extend it to add more specialized science data types like Image and Spectrum classes.

@astrofrog
Copy link
Member

@iguananaut - that's a good idea (using Quantity for NDData.data).

@Cadair
Copy link
Member

Cadair commented Aug 6, 2013

I agree that could be useful but I would suggest that it not be required. What happens if you want to use a memmap of a ndarray or a file i.e. HDF5 or FITS? The wonderful thing about NDData is that it can have any data attribute, be it a ndarray, a pandads dataframe etc.

@embray You should have a look at the SunPy map module, it is close to being a NDData specilisation for a Image. I am working on designing one for a multi-wavelength image and a timeseries' there of.

@astrofrog
Copy link
Member

@Cadair - those are very good points - we could keep NDData as simply requiring an ndarray-compatible object inside, rather than strictly an ndarray.

@Cadair
Copy link
Member

Cadair commented Aug 6, 2013

I think that would be the best solution. Obviously subclasses could be more specific.

@embray
Copy link
Member Author

embray commented Aug 6, 2013

@Cadair That was my thinking too. I think it would be often very useful to use Quantity for the NDData's data object, but there's no reason it should be strictly required (though I would say if NDData.data is a Quantity then NDData.unit should be tied to NDData.data.unit somehow.

Note also an mmap'd array can still be the base for a Quantity array too.

@embray
Copy link
Member Author

embray commented Nov 24, 2014

This issue will probably become mostly irrelevant as the purpose and scope of NDData is being reconsidered. However, I'll leave it open until it actually becomes irrelevant and/or is fixed in the new implementation.

@wkerzendorf
Copy link
Member

@embray still want to leave it open?

@embray
Copy link
Member Author

embray commented Jan 16, 2015

Hmm, well, I don't know if the NDData APE mentions this (probably not), but it might be nice if we required that when a slice returns a scalar it returns an actual scalar instead of a zero-dimensional array.

@mwcraig
Copy link
Member

mwcraig commented Jun 10, 2017

Closing, since it appears to be fixed in the latest (v1.3) nddata:

In [2]: from astropy.nddata import NDData

In [3]: d = NDData([1, 2, 3])

In [4]: d[0]
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-4-17371c6688f6> in <module>()
----> 1 d[0]

TypeError: 'NDData' object does not support indexing

In [5]: d.data[0]
Out[5]: 1

@mwcraig mwcraig closed this as completed Jun 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants