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

Update tutorial notebooks to use gammapy dev version #114

Closed
wants to merge 4 commits into from
Closed

Update tutorial notebooks to use gammapy dev version #114

wants to merge 4 commits into from

Conversation

juanep97
Copy link

@juanep97 juanep97 commented Feb 24, 2022

This PR modifies the notebooks so they are able to run with gammapy dev version.

Changes in this PR:

  • Modifies test data Mrk421_2011.ecsv and PKS1510-089_2015b.ecsv removing points with repeeated energy since they could not be loaded by gammapy; as a workaround until this possibility is implemented.
  • Modifies the editting of uncertainty, since FluxPoints instances don't have an accesible .dataattribute anymore, and it gave errors.
  • Small updates to the block of code doing the fitting.
  • Modifies the plot parameters (kwargs passed to plot) since the names had changed and they were not anymore compatible.

@cosimoNigro cosimoNigro self-assigned this Feb 28, 2022
@cosimoNigro cosimoNigro added the enhancement New feature or request label Feb 28, 2022
@codecov
Copy link

codecov bot commented Feb 28, 2022

Codecov Report

Merging #114 (087052f) into master (86a28d4) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #114   +/-   ##
=======================================
  Coverage   96.99%   96.99%           
=======================================
  Files          30       30           
  Lines        2233     2233           
=======================================
  Hits         2166     2166           
  Misses         67       67           
Flag Coverage Δ
unittests 96.99% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 86a28d4...087052f. Read the comment docs.

@cosimoNigro
Copy link
Owner

Dear @jepcor97,

apologies for replying so late and thanks a lot for the effort of updating the notebooks.

I do not like the idea of deleting data points with repeated energies in the dataset.
Now, we had this discussion with @adonath while preparing the agnpy_paper.
If an additional info is available in the table (e.g. a string with the instrument name), Axel showed how to separate the flux points belonging to each instrument and then define a single FluxPointsDataset for each of them.
cosimoNigro/agnpy_paper#18 (comment)

This would solve the issue for the PKS1510-089 dataset, for which, thanks to Julian, we solved the problem of provenance.
There was a problem with single-bin flux measurement, but this will be fixed as soon as gammapy/gammapy#3807 is merged in gammapy-master.

I contacted the authors of the Mrk421 2011 to ask if they had the MWL SED ordered by instrument, but did not receive any answer so far. I suggest we make an other attempt - I am writing to them now, and if we do not manage, then let us simply change the dataset! I am sure we can find another MWL SED, even from the same Mrk421.

As I said I do not like the idea of canceling measurements just because we cannot make flux points with them.
Also Andrea is using the same dataset in jetset, though he re-bins the flux points. I think it would be nice to keep the consistency between the two software examples.

Thanks for your work Juan.

@cosimoNigro
Copy link
Owner

cosimoNigro commented Mar 4, 2022

Since that dataset is "iconic" we can also consider keeping it and simply show an example of fit with sherpa using it.
And choose another BL Lac to fit with Gammapy.
We can differentiate the sources fitted with Gammapy and sherpa.

@juanep97
Copy link
Author

juanep97 commented Mar 7, 2022

Hi @cosimoNigro,
You are right, it would be much better to use a dataset with provenance, so we can separate them like in that link you sent. In fact, that would also automatically suply a tutorial using a composed dataset, which would illustrate the need of flattening that array I told you about through slack. When we have the new dataset, I can try to update the notebooks to it. The remaning modifications of the tutorials, some because of matplotlib and some because of gammapy dev, are still necessary, I think. Let's wait then. Thank you very much.

@cosimoNigro
Copy link
Owner

@jepcor97, I added Gammapy and sherpa wrappers in a parallel PR #119.
This PR is superseded by it.

Thanks anyhow for your work.

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

Successfully merging this pull request may close these issues.

None yet

2 participants