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

Check for and prevent initialization of NDData with Quantity #2380

Merged
merged 4 commits into from May 30, 2014

Conversation

mwcraig
Copy link
Member

@mwcraig mwcraig commented Apr 28, 2014

This PR simply raises an error if you try to initialize NDData or StdDevUncertainty with a Quantity (see #2374)

Once I know which release this is targeted for (if any--it may be completely the wrong way to go) I'll add a changelog entry.

I could, if desired, do something a little more sophisticated (and perhaps more sensible) than this, though I don't think what is below is a long-term solution:

  • Initializing NDData with a Quantity could set the NDData.data (as a numpy.ndarray) and NDData.unit
  • Initializing StdDevUncertainty with Quantity could convert that quantity to the units of StdDevUncertainty.parent_nddata then set StdDevUncertainty.array with the result, raising an error if parent_nddata is None.

@embray
Copy link
Member

embray commented Apr 28, 2014

Seems okay to me for the time being.

@astrofrog astrofrog added this to the v0.4.0 milestone Apr 30, 2014
@astrofrog
Copy link
Member

@mwcraig - can you rebase this?

@mwcraig
Copy link
Member Author

mwcraig commented May 19, 2014

This is good to go

@astrofrog
Copy link
Member

I kind of like the alternative you are suggestion to allow a quantity to be passed and do the right thing. @eteq, what do you think?

@mwcraig
Copy link
Member Author

mwcraig commented May 20, 2014

A longer-term change would be to allow (but not require) .data to be a Quantity and have .unit and perhaps the other properties have the .data object take care of those properties. A similar approach could be used to let numpy deal with the masking--allow (but again, not require) .data to be a masked array, and have NDData's mask defer to it.

Haven't really thought this through (and probably shouldn't until ccdproc is closer to ready for some external eyes), but the implementation of, say, mask, might look something like this:

def mask(self):
    try:
        return self.data.mask
    except AttributeError:
        return self._mask

@mwcraig
Copy link
Member Author

mwcraig commented May 20, 2014

To clarify, if you want the simple ability to set NDData with a Quantity in a sensible way, I can do that in the next couple of days.

@eteq
Copy link
Member

eteq commented May 20, 2014

I agree with @astrofrog that this "simple ability" would be a very useful thing - that is, you can give a Quantity to NDData either as the value or the uncertainty, and have it behave the way @mwcraig suggests in the above bullet points.

I think for the longer-term, we either don't want to use Quantity at all internally in NDData, or always use it - e.g. NDData.unit could just behave like the mask example you gave above. But to do that right we need to have the option of masked quantities, so I think at least until then we want to keep the internals of NDData and Quantity separate.

@mwcraig
Copy link
Member Author

mwcraig commented May 22, 2014

@eteq @astrofrog -- I put in the ability to initialize with a Quantity. Doing that for the uncertainty is messy because the uncertainty needs to be created before, and independently of, its associated NDData object. I got it to work by adding a ._unit attribute to StdDevUncertainty.

I decided that it was OK to use an uncertainty without units for a NDData object with units (that was the current behavior anyway) but that using an uncertainty with units for a NNData object without units should raise an error.

@eteq
Copy link
Member

eteq commented May 26, 2014

I think this behavior all makes sense and looks good to me, @mwcraig - can you update the CHANGES.rst to reflect what it's now doing?

@astrofrog, look good to you other than that?

1 similar comment
@eteq
Copy link
Member

eteq commented May 26, 2014

I think this behavior all makes sense and looks good to me, @mwcraig - can you update the CHANGES.rst to reflect what it's now doing?

@astrofrog, look good to you other than that?

@astrofrog
Copy link
Member

👍

@eteq
Copy link
Member

eteq commented May 30, 2014

Looks good. Merging!

eteq added a commit that referenced this pull request May 30, 2014
Allow initialization of NDData with Quantity
@eteq eteq merged commit bacf959 into astropy:master May 30, 2014
@mwcraig mwcraig deleted the nddata-no-qty branch June 4, 2014 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants