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

Improve spectral model uncertainty handling #885

Merged
merged 17 commits into from Feb 15, 2017

Conversation

@adonath
Copy link
Member

@adonath adonath commented Feb 11, 2017

This PR refactors the uncertainty handling in SpectralModel:

  • Add evaluate_error, integral_error and energy_flux_error methods, which offer the same API as the corresponding standard methods (including quantity handling!)
  • Refactor handling of uncertainties in SourceCatalogObjectGammaCat
  • Add plot_error method, which is currently a duplication of SpectrumButterfly.plot(). But having this method on the SpectralModel class made much more sense to me. My suggestion would even be to completely remove the SpectrumButtterfly class in a later PR, because in its current status it doesn't offer any useful functionality and doesn't follow our implementation standards (still inherits from QTable).
@adonath adonath self-assigned this Feb 11, 2017
@adonath adonath added this to the 0.6 milestone Feb 11, 2017
@adonath adonath force-pushed the adonath:refactor_uncertainty_handling branch from 1f66630 to 1d515c8 Feb 11, 2017
Copy link
Member

@cdeil cdeil left a comment

I had a quick look and left two inline comments.
Otherwise 👍 , please merge when ready.

Concerning the Butterfly class -- the reason we introduced it was to split computation from plotting and eventually also to have a serialisation format and a place to put the I/O methods. So I think we might want to keep it, or at least discuss whether to use a plain Table or our own SpectrumButterfly class before removing it. Changing it to hold a Table attribute instead of sub-classing Table would probably be a good move (not here, someone, some day) that doesn't need discussion.

# row_data = row_data.filled()
#data = OrderedDict(zip(row.colnames, row_data))
#data[self._source_index_key] = idx
#return data

This comment has been minimized.

@cdeil

cdeil Feb 11, 2017
Member

What's going on here with this uncommented code?
Maybe remove it completely?
(we still have it in git history if needed)

@@ -365,11 +423,11 @@ class SpectralModelPowerLaw(SpectralModel):

@classmethod
def from_pset(cls, pset):

This comment has been minimized.

@cdeil

cdeil Feb 11, 2017
Member

Since it's not called ParameterList, all the "pset" should be changed to "plist", no?
Can you do it here or should I in a follow-up commit?

@cdeil
Copy link
Member

@cdeil cdeil commented Feb 12, 2017

@adonath - Please merge this soon. Let's just sort out things that brake (if any) in the coming days.
(I need to continue with gamma-cat, and I think currently ./make.py all there is broken and maybe adapting to this change might fix the issue...)

@joleroi
Copy link
Contributor

@joleroi joleroi commented Feb 13, 2017

I had a look at the way you implemented the error handling in ParameterList - 👍
You did not remove SpectrumFitResult.model_with_uncertainties, should I do that?

@adonath adonath force-pushed the adonath:refactor_uncertainty_handling branch from 80c33f8 to 2f75137 Feb 14, 2017
@cdeil cdeil mentioned this pull request Feb 15, 2017
0 of 2 tasks complete
@cdeil
Copy link
Member

@cdeil cdeil commented Feb 15, 2017

Can you add this fix to this PR?
adonath#1

----------
name : str
Name of the parameter.
value : float or `~astropy.units.quantity`

This comment has been minimized.

@cdeil

cdeil Feb 15, 2017
Member

Should this really be "value: float or quantity"?
Can we make it just float to keep it simple?

This comment has been minimized.

@joleroi

joleroi Feb 15, 2017
Contributor

This is a typo. only float is accepted. For quantities there is a property with associated setter.

@adonath adonath merged commit 99579df into gammapy:master Feb 15, 2017
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@adonath
Copy link
Member Author

@adonath adonath commented Feb 15, 2017

@cdeil @king I decided to merge now and address remaining issues in a follow up PR.

@cdeil cdeil changed the title Refactor uncertainty handling in SpectralModel class Improve spectral model uncertainty handling Feb 18, 2017
@adonath adonath deleted the adonath:refactor_uncertainty_handling branch Nov 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants