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

NDData and NDDataBase Refactor #4270

Merged
merged 10 commits into from Jan 18, 2016

Conversation

Projects
None yet
4 participants
@MSeifert04
Contributor

MSeifert04 commented Oct 25, 2015

This is the refactor of NDDataBase and NDData (from #4234). More comments will follow as soon as I get the splitted parts to work.

edit 12/22/2015 to add checklist for completion:

  • Update documentation to reflect changes
  • Remove restriction on type of uncertainty_type (ping list notifying about this)
  • Always return the uncertainty object, not the data contained in that object, even if an UnknownUncertainty is created by NDData

@MSeifert04 MSeifert04 changed the title from Initial refactor of NDData and NDDataBase to NDData and NDDataBase Refactor Oct 25, 2015

@MSeifert04

This comment has been minimized.

Contributor

MSeifert04 commented Oct 25, 2015

Summary:
__init__:

  • New optional copy parameter. The default is False which was the case before this PR.
  • Implicit parameters (i.e. data parameter is another NDData instance) are always checked. This might have produced Bugs if subclasses had less requirements.
  • data can now be a Masked_Quantities instance
  • data with mask is only allowed if they have also a data attribute (Duck typing MaskedArrays)
  • If an implicit and explicit parameter is given (i.e. data was a Quantity and a unit was given) the implicit one is ignored and a warning is raised! Before this PR that was not always done, i.e. two different units raised an Exception while two different masks raised a Warning.

uncertainty

  • The uncertainty setter of NDDataBase has moved to NDData
  • uncertainties get their parent_nddata attribute set in the setter. (see issue #4152)
  • The uncertainty setter now checks for an attribute uncertainty_type and if it is not present or not a string the uncertainty is wrapped in an UnknownUncertainty. (Before it raised an Exception)
  • The getter uncertainty special cases UnknownUncertaintiy and returns only what was initially wanted as uncertainty but the internally stored _uncertainty is of unknown uncertainty_type.

I think this is mostly added functionality and should be backwards compatible, except for the uncertainty setter. Subclasses of NDDataBase might encounter some transitional problems since the one is now gone. All other changes only lessened restrictions (Warning instead of Exceptions).

But I now know, that some of these changes will be somehow controversial so I would be very happy about comments.

return self._uncertainty
@property
def uncertainty_type(self):

This comment has been minimized.

@mwcraig

mwcraig Oct 26, 2015

Contributor

Is the idea here that eventually every NDData object would have an uncertainty, defaulting to UnknownUncertainty, eventually? Otherwise it seems like leaving the uncertainty_type a property of the uncertainty would be easier.

This comment has been minimized.

@mwcraig

mwcraig Oct 26, 2015

Contributor

Ah, right, UnknownUncertainty is at the top of the file.

This comment has been minimized.

@MSeifert04

MSeifert04 Oct 27, 2015

Contributor

The uncertainty_type property was just a convenience I used while I tested the UnknownUncertainty. I thought I deleted it before pushing - but I forgot.

@mwcraig mwcraig added the nddata label Oct 26, 2015

@mwcraig

This comment has been minimized.

Contributor

mwcraig commented Oct 26, 2015

@wkerzendorf -- could you please take a look at this one? Looks mostly straightforward to me, the one big change is to wrap the uncertainty in an UnknownUncertainty if the uncertainty does not have an attribute uncertainty_type.

@mwcraig

This comment has been minimized.

Contributor

mwcraig commented Oct 26, 2015

@MSeifert04 -- once again, thanks. I gave this a quick look tonight and didn't see any problems with the code, but I'm not sure about dropping the requirement that the uncertainty have an uncertainty_type (primarily because the APE currently says it should be there).

It sounds, though, like your solution is to wrap an uncertainty that does not have an uncertainty_type in an UnkownUncertainty, right? So in the end there is always an uncertainty_type?

@MSeifert04

This comment has been minimized.

Contributor

MSeifert04 commented Oct 27, 2015

@mwcraig - Yes, your summary about the UnknownUncertainty is correct.
But note that I also changed the uncertainty getter so that for UnknownUncertainty it doesn't return the instance but only the saved array of the instance. So a normal user would not suspect that the uncertainty was modified (except in some cases, i.e. the uncertainty was a list and cast to a np.ndarray). It felt a bit like cheating to be honest and if you don't like it I can revert back to the former enforced requirement.

I got the idea from #3714 where @wkerzendorf proposed just such an uncertainty and I modified it a bit to match my NDUncertainty changes.

Thanks for looking over it!

@mwcraig

This comment has been minimized.

Contributor

mwcraig commented Oct 27, 2015

I'll have some time tomorrow to take a longer look at the others, and to put a call out on the astropy-dev list for code reviews.

I know a bunch of people are at ADASS this week...

@mwcraig mwcraig referenced this pull request Dec 5, 2015

Merged

NDArithmeticMixin and NDUncertainty Refactor #4272

0 of 3 tasks complete
saved in _uncertainty has an uncertainty_type but it's unknown :-)
"""
if isinstance(self._uncertainty, UnknownUncertainty):
return self._uncertainty.array

This comment has been minimized.

@mwcraig

mwcraig Dec 22, 2015

Contributor

Recommend switching this back to return self._uncertainty

@MSeifert04

This comment has been minimized.

Contributor

MSeifert04 commented Dec 22, 2015

In the Hangouts meeting @wkerzendorf and @mwcraig suggested some changes. The current push reflects these:

  • UnknownUncertainty is handled like any other uncertainty when accessed via the uncertainty property.
  • uncertainty_type needs to be present for uncertainty.setter but we dropped the requirement for it to be a string.

I skipped the documentation update for now.

@mwcraig

This comment has been minimized.

Contributor

mwcraig commented Jan 6, 2016

@MSeifert04 -- could you please go ahead and do the documentation changes too (should be minimal, I think, for this one)?

@MSeifert04

This comment has been minimized.

Contributor

MSeifert04 commented Jan 6, 2016

@mwcraig - I edited the documentation files to reflect the changes and included a section with a summary of the changes. I'm not very happy with the wording, just let me know if anything should be edited.

`~astropy.nddata.NDDataBase`:
+ `NDDataBase.uncertainty.getter`: is now abstract.

This comment has been minimized.

@mwcraig

mwcraig Jan 7, 2016

Contributor

This and the line below are causing the sphinx failures because there is no getter or setter method.

This comment has been minimized.

@mwcraig

mwcraig Jan 7, 2016

Contributor

Won't be an issue, though, if this just moves to the changelog.

uncertainty_type: ``"unknown"``
The type of uncertainty is unknown.
Notes

This comment has been minimized.

@mwcraig

mwcraig Jan 7, 2016

Contributor

Could remove this note (or move it to a comment in the class body).

# Check if meta is a dict and create an empty one if no meta was given
if meta is None:
meta = OrderedDict()
elif not isinstance(meta, collections.Mapping):

This comment has been minimized.

@mwcraig

mwcraig Jan 7, 2016

Contributor

nifty - hadn't realized collections.Mapping could be used to check for a dict-like interface.

The actual data contained in this `NDData` object. If possible, data
will not be copied`data`, so you should make copy the ``data`` before
passing it in if that's the desired behavior.
data : `~numpy.ndarray`-like, `NDData`-like or list

This comment has been minimized.

@mwcraig

mwcraig Jan 7, 2016

Contributor

Can drop list, it is array-like already.

# Rather pointless since the NDDataBase does not implement any setting
# but before (#4234) the NDDataBase did call the uncertainty

This comment has been minimized.

@mwcraig

mwcraig Jan 7, 2016

Contributor

Drop the reference to #4234

classes.
NDDataBase makes no attempt of restricting any attribute or defines how
these are stored. Therefore all properties and methods have to be
overridden in subclasses. See `~astropy.nddata.NDData` for a subclass that

This comment has been minimized.

@mwcraig

mwcraig Jan 7, 2016

Contributor

Suggested edit: drop the sentence that begins "NDDataBase makes...".

Change "Therefore all" -> "All"

from ...tests.helper import pytest, raises
from ...utils.compat.odict import OrderedDict
from ..nduncertainty import StdDevUncertainty # , NDUncertainty
from ...tests.helper import pytest # , raises

This comment has been minimized.

@mwcraig

mwcraig Jan 7, 2016

Contributor

Please drop the unused imports completely rather than making them comments (and thanks for the clean-up).

assert ndd.data[1] != quantity.value[1]
def test_nddata_init_data_maskedQuantity():

This comment has been minimized.

@mwcraig

mwcraig Jan 7, 2016

Contributor

Pedantic naming issue: maskedQuatity -> masked_quantity

# Now let's see what happens if we have all explicitly set
nd1 = NDData(np.array([1]), mask=False, uncertainty=10, unit=u.s,
meta={'dest': 'mordor'}, wcs=10)

This comment has been minimized.

@mwcraig

mwcraig Jan 7, 2016

Contributor

Nice meta :)

assert nd3.meta != nd1.meta
def test_nddata_init_data_nddataSubclass():

This comment has been minimized.

@mwcraig

mwcraig Jan 7, 2016

Contributor

nddataSubclass -> nddata_subclass

@@ -5,7 +5,7 @@
unicode_literals)
from ..nddata_base import NDDataBase
from ...tests.helper import pytest
# from ...tests.helper import pytest

This comment has been minimized.

@mwcraig

mwcraig Jan 7, 2016

Contributor

Remove instead of commenting out if unneeded.

@@ -24,7 +24,8 @@ datasets in astropy through:
.. warning::
`~astropy.nddata` has changed significantly in astropy 1.0. See the section
:ref:`nddata_transition` for more information.
:ref:`nddata_transition` for more information. Some more changes were done in
astropy x.xx, see more details in :ref:`nddata_transition2`.

This comment has been minimized.

@mwcraig

mwcraig Jan 7, 2016

Contributor

I think we could take out this warning box altogether...it has been (almost?) a year since 1.0 was released.

.. _nddata_transition2:
Transition to astropy x.xx
==========================

This comment has been minimized.

@mwcraig

mwcraig Jan 7, 2016

Contributor

I think this should be in the changelog rather than a separate section in the sphinx documentation. All of the detailed changes show up in the docstrings and examples, and

@mwcraig

This comment has been minimized.

Contributor

mwcraig commented Jan 7, 2016

@MSeifert04 -- looks really good, just a few minor inline comments. Most of the bullet points you added to index.rst should go in CHANGES.rst instead -- please put them in the section for version 1.2.

by default False. If it is True all other parameters are copied before saving
them as attributes. If False the parameters are only copied if there is no
way of saving them as reference.
+ the ``data`` parameter allows now also ``Masked_Quantity`` objects

This comment has been minimized.

@mwcraig

mwcraig Jan 7, 2016

Contributor

I don't think Quantity can be masked yet?

@mwcraig

This comment has been minimized.

Contributor

mwcraig commented Jan 11, 2016

@MSeifert04 -- looking good, just some minor changes in the changelog entries.

@MSeifert04

This comment has been minimized.

Contributor

MSeifert04 commented Jan 11, 2016

Apart from the changes, I just found that there is a descriptor lingering in the utils for the meta: https://github.com/astropy/astropy/blob/master/astropy/utils/metadata.py#L115

Since this is completly identical to the meta handling (except that that it implements a setter which isn't present until now) in NDData: Should I include it here?

Would look something like this

from astropy.utils.metadata import MetaData
class NDData(NDDataBase):
    meta = MetaData()
    ...
@wkerzendorf

This comment has been minimized.

Member

wkerzendorf commented Jan 11, 2016

I think this is a very good idea. and you can subclass this to make the arithmetic operations on them.

@wkerzendorf

This comment has been minimized.

Member

wkerzendorf commented Jan 11, 2016

But this PR does not deal with meta, right? Did we merge one of them?

@mwcraig

This comment has been minimized.

Contributor

mwcraig commented Jan 11, 2016

@MSeifert04 -- could you please open a separate issue to remind us to look at moving metadata to the one in utils?

That way we can get this merged sooner but still remember we need to come back to the issue.

@MSeifert04

This comment has been minimized.

Contributor

MSeifert04 commented Jan 12, 2016

@mwcraig I've opened a seperate issue and will make a seperate PR as soon as this is merged.

@mwcraig

This comment has been minimized.

Contributor

mwcraig commented Jan 17, 2016

@MSeifert04 -- looks like this just needs a rebase (probably related to the changelog) then it is ready to merge.

@MSeifert04 MSeifert04 force-pushed the MSeifert04:nddata_nddatabase branch from 9c79aea to 0f361a7 Jan 18, 2016

@MSeifert04

This comment has been minimized.

Contributor

MSeifert04 commented Jan 18, 2016

@mwcraig - My first rebase - I hope I did not screw it up. The rebase was because of #4466.

nd = NDData([1, 2, 3], meta={})
assert len(nd.meta) == 0
nd = NDData([1, 2, 3])
assert isinstance(nd.meta, OrderedDict)

This comment has been minimized.

@mwcraig

mwcraig Jan 18, 2016

Contributor

This is where the test failures are happening:

_______________________________ test_param_meta ________________________________
    def test_param_meta():
        # everything dict-like is allowed
        with pytest.raises(TypeError):
            NDData([1], meta=3)
        nd = NDData([1, 2, 3], meta={})
        assert len(nd.meta) == 0
        nd = NDData([1, 2, 3])
>       assert isinstance(nd.meta, OrderedDict)
E       assert isinstance(OrderedDict(), OrderedDict)
E        +  where OrderedDict() = NDData([1, 2, 3]).meta
@@ -11,31 +11,41 @@
from numpy.testing import assert_array_equal
from ..nddata import NDData
from ..nduncertainty import StdDevUncertainty, NDUncertainty
from ...tests.helper import pytest, raises
from ...utils.compat.odict import OrderedDict

This comment has been minimized.

@mwcraig

mwcraig Jan 18, 2016

Contributor

Am guessing this needs to change now that the compat odict is gone?

This comment has been minimized.

@astrofrog

This comment has been minimized.

@MSeifert04

MSeifert04 Jan 18, 2016

Contributor

Alright, I'll fix it later. Thanks for tracking it down.

This comment has been minimized.

@MSeifert04

MSeifert04 Jan 18, 2016

Contributor

@mwcraig and @astrofrog Thanks - I've pushed a fix.

@mwcraig

This comment has been minimized.

Contributor

mwcraig commented Jan 18, 2016

Note to self: also checked this with ccdproc and nothing broke...

@mwcraig

This comment has been minimized.

Contributor

mwcraig commented Jan 18, 2016

🎉 Thanks again for this @MSeifert04!

mwcraig added a commit that referenced this pull request Jan 18, 2016

Merge pull request #4270 from MSeifert04/nddata_nddatabase
NDData and NDDataBase Refactor

@mwcraig mwcraig merged commit a2391d0 into astropy:master Jan 18, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.01%) to 76.619%
Details

MSeifert04 added a commit to MSeifert04/astropy that referenced this pull request Jan 27, 2016

@MSeifert04 MSeifert04 deleted the MSeifert04:nddata_nddatabase branch Feb 28, 2016

dhomeier added a commit to dhomeier/astropy that referenced this pull request Jun 12, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment