Make initialization of results in arithmetic and unit conversion to allow more flexibility in subclasses

[mwcraig]

This PR would close #2300

[embray]

I think this looks alright. What we might want to lean toward in the future, which is trick the pickle library uses for some cases like this, is to actually provide a method that subclasses can override which returns their "init args"--basically any positional and keyword arguments needed to initialize a new instance of class. That way subclasses aren't also tied to the signature of the base class's __init__ here.

[mwcraig]

👍 More flexibility would be great.

[eteq]

@wkerzendorf @astrofrog - one or both of you may want to look at this, but it looks fine to me as a measure to make things more flexible now.

@embray may be right that we want to generalize this more, but I think we need to see a few more use-cases before it's worth the effort. E.g., if CCDImage is the only regularly-used subclass, then this is fine as-is...

Oh, and minor tip, @mwcraig : If you put closes #2300 in the commit log, it will auto-close #2300 when this is merged. I'm fairly sure it doesn't work if you say it in the github comments, though (as you have above).

[mwcraig]

@eteq -- will try to get in the habit of putting closes in the commit log, but it does work from the pull request body too (though not the PR title): https://help.github.com/articles/closing-issues-via-commit-messages

[astrofrog]

@mwcraig - looks good to me - could you add a unit test?

[mwcraig]

@astrofrog -- tests added!

[eteq]

Nice, @mwcraig - didn't know that (or maybe I did and forgot...). I guess I was thinking of titles, then. Not terribly important to be in the commit log, then - only very mild preference.

Looks good to me, as the tests are now passing. Lets give @wkerzendorf another day or two to answer if he has a chance, otherwise we can go ahead and merge this?

[wkerzendorf]

@mwcraig I think that looks like it fixes it - job well done (sorry for delayed response - am travelling).

[eteq]

Alright, great, then I'll merge. Thanks @mwcraig !

[mdboom]

Hmm... I would have just replaced the data member with a Quantity instance and taken the unit member away. Then you can just assert that data is a Quantity.

[mwcraig]

@mdboom -- does NDData support Quantity as data? My reading of the docs was that it didn't. OTOH, if I try nd = NDData([1,2,3] * u.second) then the only things that seem broken after a cursory look are:

• repr(nd) (raises a TypeError)
• nd.unit is completely unrelated to nd.data.unit (taking the unit member away wouldn't work in the pre-merge version of _arithmetic, I think, because it created result and several lines later set result.unit; also, some of the code in _arithmetic uses the unit member of the operands to do units checks)

[mdboom]

Sorry I wasn't clear. Quantity as the data member of NDData isn't supported now, but I was wondering out loud if it could be. (NDData predates Quantity). It would require a number of changes (most importantly turning the unit property into something that delegates to the underlying property), but it might be doable, and might resolve #2300.

[wkerzendorf]

At some stage I had the idea that NDData could inherit from Quantity.
That would solve quite a few problems. Maybe now that Quantity is more
stable that might be an idea again.

On Thu, Apr 17, 2014 at 8:16 PM, Michael Droettboom <
notifications@github.com> wrote:

Sorry I wasn't clear. Quantity as the data member of NDData isn't
supported now, but I was wondering out loud if it could be. (NDDatapredates
Quantity). It would require a number of changes (most importantly turning
the unit property into something that delegates to the underlying
property), but it might be doable, and might resolve #2300#2300
.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/2301#issuecomment-40745089
.