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

Added check for NDData.uncertainty being StdDevUncertainty array in construction of EPSFStar object #739

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Onoddil
Copy link
Contributor

@Onoddil Onoddil commented Oct 8, 2018

Added the option for the weights in EPSFStar objects to be passed from StdDevUncertainty NDData arrays, extending from the original weights uncertainty_type option. Also added brief comment describing the origin of weights in EPSFFitter fitter. Closes #734

@Onoddil Onoddil closed this Oct 9, 2018
@Onoddil Onoddil reopened this Oct 9, 2018
@Onoddil Onoddil force-pushed the nddata_uncertainty_description branch from 940f0a7 to 68b13fe Compare October 17, 2018 14:24
Copy link
Member

@eteq eteq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An informational note and one minor change (which I think you can just hit "accept" on), otherwise looks good.

# the weight. Before assigning the weights we must ignore zero
# variance data, however, setting the weights for these data points to
# zero by forcing their standard deviations to a large value.
elif data.uncertainty.uncertainty_type == 'std':
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that in the upcoming astropy release (3.1) it looks to me like there will be an easier way to "normalize" other uncertainties into std. We'll want to do that for this option I'd think, but no point in doing that until the astropy release is available for testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So is this a "accept this and we'll revisit it later" or a "hold off on this until after the release is available for testing"? I think it would be good in the future to expand this quite a fair amount to accept various kinds of uncertainty (perhaps parsing them through some convenience function to produce an internally accepted 'standard' type), and it sounds like this will help with that down the line.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Astropy 3.1 is out, so I've assigned @mwcraig to this issue in case he can help with this :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now what you could is something like this:

uncertainty_conversion_to_std = {
     'std': lambda x: x,  # do nothing
     'var': np.sqrt,  # square root of the variance is the standard dev
     'ivar': lambda x: 1 / np.sqrt(x)   # invert and square root to get standard dev
}

uncert = data.uncertainty.array.copy()
std_dev = uncertainty_conversion_to_std[data.uncertainty.type](uncert)

(note: I haven't tested this code but hopefully it conveys the idea).

CHANGES.rst Outdated Show resolved Hide resolved
Onoddil and others added 4 commits April 3, 2019 12:43
… creating EPSFStar instance via extract_stars. Also added description comment to EPSFFitter.fit_star detailing where fitter weights originate.
@Onoddil Onoddil force-pushed the nddata_uncertainty_description branch from 48f1964 to 19bbc6d Compare April 3, 2019 16:51
@Onoddil
Copy link
Contributor Author

Onoddil commented Apr 8, 2019

Have made the changes requested @eteq - I note however that astropy 3.1 has been released subsequently and therefore the new changes to uncertainties must be updated; I think it makes sense to merge this PR as a base to build the new functionality for std normalisation onto, but if that should be built into this PR before acceptance then let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ePSF weights are passed as data.uncertainty
5 participants