Skip to content

Conversation

@crawfordsm
Copy link
Member

While trying out the nddata and nduncertainity classes, I noticed that the propagation of errors for multiple/divide did not work. The problem was that in propagate_divide and propagate_multiply they were calling a self.data that didn't exist. To fix this problem, I changed self.data to self.parent_nddata.data in those two tasks. I also added 'self._parent_nddata=value' to the setter function for parent_nddata.

In addition, I added two tests to test_nddata to test for propogation of errors when using multiply and divide.

@crawfordsm
Copy link
Member Author

@wkerzendorf @eteq Hi Erik and Wolfgang, just wanted to pull your attention to this. This error came up when I was trying nddata out while working on the ccdproc package. It looks good to merge, but I could also think that someone would want to fix it in another way

@wkerzendorf
Copy link
Member

@crawfordsm I should warn you however that the nduncertainty part of nddata is not stable and is likely being revised with an uncertainty formalism that pertains to both Quantity and NDData.

As for this PR, I need to have a look through, referencing potentially big arrays from another class is sometimes a recipe for memory leaks.

@crawfordsm
Copy link
Member Author

Thanks @wkerzendorf ! Please definitely check it out. I understand things will be updating but the current divide and multiple in stduncertainity were not working due to calling self.data that didn't exist. Also, _parent_nddata was never being assigned. So there were the only two things I updated so that it did actually work. However, I definitely understand there might be a better way to do this and I'm happy to open up an issue and delete this PR if there is a better way

@eteq
Copy link
Member

eteq commented Oct 18, 2013

I think this is exactly what was intended, it's just a bug, because the uncertainty shouldn't have a .data. I don't think this is a memory leak any more than the whole idea of NDUncertainty is. So unless @wkerzendorf specifically says this isn't supposed to work this way, I'll merge this soon.

@wkerzendorf
Copy link
Member

@eteq I think for now it's fine. As discussed in the meeting, the treatment of uncertainties will require a good amount of effort on multiple fronts. However, small implementation like this might teach us how its used. Thanks for looking into this @crawfordsm!

@eteq
Copy link
Member

eteq commented Oct 18, 2013

Great, I'll go ahead and merge this, then. Thanks @crawfordsm !

eteq added a commit that referenced this pull request Oct 18, 2013
update nduncertainty for multiple/divide
@eteq eteq merged commit b92c53b into astropy:master Oct 18, 2013
@embray
Copy link
Member

embray commented Nov 12, 2013

Hadn't noticed this before--it should have been backported to the 0.2.x branch, but that's okay. In the remote chance that it becomes required we can backport it, but that seems doubtful once 0.3 is out.

@crawfordsm crawfordsm deleted the uncertainty_divide branch January 19, 2016 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants