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

Add a norm parameter to the EBL model #2454

Merged
merged 4 commits into from Oct 14, 2019
Merged

Conversation

JouvinLea
Copy link

This PR adds a norm to the EBL model to give the possibility to the user to add a norm for the AbsorbedSpectralModel class and also to let it free in the fits.

It is also adding a small argument for the peek() method of FLuxPointDataset because for now the uncertainties are not handle properly and it crashed if you try to plot the error of this model.

@JouvinLea JouvinLea requested review from QRemy, registerrier and santiagopita and removed request for QRemy October 11, 2019 14:43
@cdeil cdeil self-assigned this Oct 11, 2019
@cdeil cdeil added the feature label Oct 11, 2019
@cdeil cdeil added this to the 0.15 milestone Oct 11, 2019
@cdeil cdeil added this to In progress in gammapy.modeling via automation Oct 11, 2019
Copy link
Contributor

@cdeil cdeil left a comment

Choose a reason for hiding this comment

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

@JouvinLea - Thanks!

Currently it's not really clear how the AbsorbedSpectralModel uses Absorption:
https://docs.gammapy.org/0.14/api/gammapy.modeling.models.AbsorbedSpectralModel.html
Now that there is an alpha parameter, it's even less clear.
@JouvinLea - Maybe you could add the formula that is applied, and that would make it crystal-clear already, without needing much description?
Is there a good literature reference with this formula for absorption?
Even if it's simple and standard, I think for people like me that have never looked at EBL stuff it would be useful.

I'm not a fan of the plot_error option you're adding. IMO this not working should be considered a bug in Gammapy and we should just fix it (#2255). Would prefer not to add such workarounds in Gammapy. But OK, if you want it, because those plotting functions are important to you for this model, then we can merge this in, given that it's not clear if / when / who will fix this.

gammapy/modeling/models/spectral.py Outdated Show resolved Hide resolved
gammapy/modeling/models/spectral.py Outdated Show resolved Hide resolved
gammapy/modeling/tests/test_serialize_yaml.py Outdated Show resolved Hide resolved
gammapy/spectrum/flux_point.py Outdated Show resolved Hide resolved
gammapy/spectrum/flux_point.py Outdated Show resolved Hide resolved
@JouvinLea JouvinLea force-pushed the change_for_ebl branch 2 times, most recently from eb5a195 to f6f3dcc Compare October 13, 2019 09:47
@JouvinLea
Copy link
Author

@cdeil
I fixed the small docs and space issues.
I added a description for the Adsorbed model, hope it is clearer!

Personally I would prefer to stay with this plot_error parameter. I put a TODO to say to remove it when the issue is fixed. I know exactly which line to comment to have my script running but this is a useful plot routine so I think for non developper it is better to have this option. But as you wish, I can comments those lines by myself each time I run my script.

cdeil
cdeil previously approved these changes Oct 14, 2019
@cdeil
Copy link
Contributor

cdeil commented Oct 14, 2019

@JouvinLea - Thanks!

@cdeil cdeil merged commit 5039146 into gammapy:master Oct 14, 2019
gammapy.modeling automation moved this from In progress to Done Oct 14, 2019
@adonath adonath changed the title Add a norm to the EBL model Add a norm parameter to the EBL model Nov 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants