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 Fermi PSF dataset and example #191

Merged
merged 7 commits into from Sep 21, 2014

Conversation

Projects
None yet
3 participants
@ellisowen
Contributor

ellisowen commented Aug 27, 2014

Adds some Fermi-LAT PSF data.

This will be used in unit tests for the FermiGalacticCenter and FermiVelaRegion psf files. I will add these tests in another PR once this one is OK & merged.

* `PSF_P7REP_SOURCE_V15 <http://www.slac.stanford.edu/exp/glast/groups/canda/archive/p7rep_v15/lat_Performance_files/cPsfEnergy_P7REP_SOURCE_V15.png>`_
* `PSF_P7SOURCEV6 <http://www.slac.stanford.edu/exp/glast/groups/canda/archive/pass7v6/lat_Performance_files/cPsfEnergy_P7SOURCE_V6.png>`_

This comment has been minimized.

@ellisowen

ellisowen Aug 27, 2014

Contributor

I couldn't find a way to keep these lines below the 80 character limit and still build OK in the docs. Do you know how I can do this?

@ellisowen

ellisowen Aug 27, 2014

Contributor

I couldn't find a way to keep these lines below the 80 character limit and still build OK in the docs. Do you know how I can do this?

This comment has been minimized.

@cdeil

cdeil Aug 28, 2014

Member

That's OK ... it's an exception ... braking the URL would be bad.

@cdeil

cdeil Aug 28, 2014

Member

That's OK ... it's an exception ... braking the URL would be bad.

Show outdated Hide outdated gammapy/datasets/fermi.py Outdated
Show outdated Hide outdated gammapy/datasets/fermi.py Outdated
@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Aug 28, 2014

Member

Please add a tutorial that computes the containment radii using the EnergyDependentTablePSF class and plots the curves R68 and R95 as a function of energy and compares to the tables you have here.

Member

cdeil commented Aug 28, 2014

Please add a tutorial that computes the containment radii using the EnergyDependentTablePSF class and plots the curves R68 and R95 as a function of energy and compares to the tables you have here.

Show outdated Hide outdated gammapy/datasets/fermi.py Outdated
@ellisowen

This comment has been minimized.

Show comment
Hide comment
@ellisowen

ellisowen Aug 29, 2014

Contributor

To do (Ellis):

  • Include FermiVelaRegion PSF in plots.
  • Add some short text to the tutorial.
Contributor

ellisowen commented Aug 29, 2014

To do (Ellis):

  • Include FermiVelaRegion PSF in plots.
  • Add some short text to the tutorial.
@ellisowen

This comment has been minimized.

Show comment
Hide comment
@ellisowen

ellisowen Aug 29, 2014

Contributor

OK, this is now ready for review.

Contributor

ellisowen commented Aug 29, 2014

OK, this is now ready for review.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Aug 31, 2014

Member

This is the result I get:
image

In the tutorial you should mention if the values make sense or not.
The Vela PSF uses P7REP_CLEAN_V15 cuts ... did you not have those available for comparison or why did you read off the P7REP_SOURCE_V15 values.
If the plot for P7REP_SOURCE_V15 is not available online, you should at least mention that P7REP_CLEAN_V15 is very similar to P7REP_SOURCE_V15 (I think, but don't know how close it actually is), so this PSF looks OK.

For the Galactic center PSF it says here that I used P7REP_CLEAN_V15, but the gtmktime and gtpsf commands are not shown there, so I suspect that I used a different IRF instead.
Looks like you or I have to replace the Galactic center dataset at some point with something more reproducible ... for now you could simply write this as a comment that this PSF is not consistent.

Member

cdeil commented Aug 31, 2014

This is the result I get:
image

In the tutorial you should mention if the values make sense or not.
The Vela PSF uses P7REP_CLEAN_V15 cuts ... did you not have those available for comparison or why did you read off the P7REP_SOURCE_V15 values.
If the plot for P7REP_SOURCE_V15 is not available online, you should at least mention that P7REP_CLEAN_V15 is very similar to P7REP_SOURCE_V15 (I think, but don't know how close it actually is), so this PSF looks OK.

For the Galactic center PSF it says here that I used P7REP_CLEAN_V15, but the gtmktime and gtpsf commands are not shown there, so I suspect that I used a different IRF instead.
Looks like you or I have to replace the Galactic center dataset at some point with something more reproducible ... for now you could simply write this as a comment that this PSF is not consistent.

Show outdated Hide outdated docs/tutorials/fermi_psf/plot_68.py Outdated
Show outdated Hide outdated docs/tutorials/fermi_psf/plot_95.py Outdated
Show outdated Hide outdated gammapy/datasets/fermi.py Outdated
@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Aug 31, 2014

Member

This is mostly OK ... some inline comments above.

The main discovery here is that I screwed up the Galactic center reference dataset.

Could you please update exposure.sh (and rename to make.sh) and README and fermi_counts.fits.gz and fermi_exposure.fits.gz and psf.fits files for the Galactic center dataset (and adapt tests as needed.
You can use the Vela ROI make.sh ... the only reason I don't include that in the gammapy repo is that it is 5 MB total, so we still need the smaller Galactic center dataset here.

Great that you found out what's wrong!

Member

cdeil commented Aug 31, 2014

This is mostly OK ... some inline comments above.

The main discovery here is that I screwed up the Galactic center reference dataset.

Could you please update exposure.sh (and rename to make.sh) and README and fermi_counts.fits.gz and fermi_exposure.fits.gz and psf.fits files for the Galactic center dataset (and adapt tests as needed.
You can use the Vela ROI make.sh ... the only reason I don't include that in the gammapy repo is that it is 5 MB total, so we still need the smaller Galactic center dataset here.

Great that you found out what's wrong!

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Aug 31, 2014

Member

And would you mind putting the four txt files into a single FITS files with four table extensions?

You can do this once interactively with a few commands in an IPython session or write 10 lines of script to do it and put it in the dev folder like I did here for example.

Member

cdeil commented Aug 31, 2014

And would you mind putting the four txt files into a single FITS files with four table extensions?

You can do this once interactively with a few commands in an IPython session or write 10 lines of script to do it and put it in the dev folder like I did here for example.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 17, 2014

Coverage Status

Coverage increased (+0.43%) when pulling d529946 on ellisowen:psf_data into a0b99ec on gammapy:master.

coveralls commented Sep 17, 2014

Coverage Status

Coverage increased (+0.43%) when pulling d529946 on ellisowen:psf_data into a0b99ec on gammapy:master.

@ellisowen

This comment has been minimized.

Show comment
Hide comment
@ellisowen

ellisowen Sep 17, 2014

Contributor

OK, this is now ready.

I didn't change the FermiGalacticCenter files in this PR, just updated the descriptions so the correct IRFs are mentioned. I don't think the updating of the datafiles themselves with P7REP_CLEAN_V15 will be something I'll have time to do myself in the next day or two, so perhaps these can just be put as an issue?

Contributor

ellisowen commented Sep 17, 2014

OK, this is now ready.

I didn't change the FermiGalacticCenter files in this PR, just updated the descriptions so the correct IRFs are mentioned. I don't think the updating of the datafiles themselves with P7REP_CLEAN_V15 will be something I'll have time to do myself in the next day or two, so perhaps these can just be put as an issue?

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Sep 17, 2014

Member

Can you please rebase this against master?

Member

cdeil commented Sep 17, 2014

Can you please rebase this against master?

@ellisowen

This comment has been minimized.

Show comment
Hide comment
@ellisowen

ellisowen Sep 17, 2014

Contributor

Ok, done!

Contributor

ellisowen commented Sep 17, 2014

Ok, done!

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Sep 21, 2014

Member

Thanks!

Member

cdeil commented Sep 21, 2014

Thanks!

cdeil added a commit that referenced this pull request Sep 21, 2014

@cdeil cdeil merged commit 55afe0d into gammapy:master Sep 21, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@cdeil cdeil changed the title from Psf data to Add Fermi PSF example Apr 8, 2015

@cdeil cdeil changed the title from Add Fermi PSF example to Add Fermi PSF dataset and example Apr 8, 2015

@cdeil cdeil added the feature label Apr 8, 2015

@cdeil cdeil added this to the 0.2 milestone Apr 8, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment