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 IRF write methods #1211

Merged
merged 11 commits into from Nov 25, 2017

Conversation

Projects
None yet
4 participants
@thomasarmstrong
Contributor

thomasarmstrong commented Nov 16, 2017

Related to issue #1157. Added features to EffectiveAreaTable2D, EnergyDispersion2D and Background3D which enable a Bintable HDU to be constructed in the current CTA IRF format (or at least related to the format specified in https://github.com/open-gamma-ray-astro/gamma-astro-data-formats). It is rather simple at the moment and could be improved with versioning, as mentioned by @kosack, along with any other suggestions. Additionally I need to work on the PSF classes a little bit as the data format isn't quite compatible. just need to have theta_lo and theta_hi instead of just offset, and the possibility of writing out additional PSF dimensions? ( CTA IRF currently has the tables SCALE, SIGMA_1, AMPL_2, SIGMA_2, AMPLE_3, SIGMA_3). I will work on this in the next couple of days.

Thomas Armstrong and others added some commits Nov 15, 2017

Thomas Armstrong
Added write functionality to IRF classes #1157
Added a function (to_table) to EffectiveAreaTable2D, EnergyDispersion2D and Background3D to allow (CTA) IRFs
to be writen out to fits files. It converts the stored data to bintableHDU which, in combination with each class
can be saved to one fits file. Still need to implement this for the PSF classes.
Small updates for write functionality to IRF classes #1157
Cleaned up some bits in the code (removing commented lines etc)
Added external changes to commit
added the files that were changed on the master branch

@cdeil cdeil self-assigned this Nov 16, 2017

@cdeil cdeil added the feature label Nov 16, 2017

@cdeil cdeil added this to the 0.8 milestone Nov 16, 2017

Show outdated Hide outdated gammapy/irf/background.py Outdated
@cdeil

I've had a quick look and left some inline comments.

This looks very good already and is a great addition!

But we first have to discuss where to use astropy.io.fits.BinTable or astropy.table.Table, and whether to use astropy.io.fits.Header or collections.OrderedDict for metadata (i.e. what Table uses).

There's pros and cons. My suggestion would be to use Table and OrderedDict for meta as much as possible, and to only go to fits.BinTable at the very last minute on FITS read / write (or explicitly via http://docs.astropy.org/en/stable/io/fits/api/tables.html#table-to-hdu).

This has the advantage of a cleaner and better code (Table is much better) and we can then also add e.g. HDF5 or other IRF serialisation in addition to FITS.
But for metadata it's a lossy conversion from astropy.io.fits.Header to collections.OrderedDict, one looses e.g. the comment for the header keys.

@thomasarmstrong @kosack - Do you have an opinion here or already a strategy / policy in CTApipe on this topic?

Show outdated Hide outdated gammapy/irf/tests/test_energy_dispersion.py Outdated
Show outdated Hide outdated gammapy/irf/energy_dispersion.py Outdated
Show outdated Hide outdated gammapy/irf/background.py Outdated
@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Nov 16, 2017

Member

Additionally I need to work on the PSF classes a little bit as the data format isn't quite compatible.

I would suggest to do that in a separate PR and keep this one limited to AEFF, EDISP and BACKGROUND.

Note that http://docs.gammapy.org/en/latest/api/gammapy.irf.PSF3D.html (the PSF class that's most important and probably the one to work on first) hasn't been changed to use NDDataArray yet, so doing that change will likely lead to quite a bit of work of fixing up callers and high-level anlaysis code, i.e. something best done separate from a PR with the main focus on FITS I/O.

Member

cdeil commented Nov 16, 2017

Additionally I need to work on the PSF classes a little bit as the data format isn't quite compatible.

I would suggest to do that in a separate PR and keep this one limited to AEFF, EDISP and BACKGROUND.

Note that http://docs.gammapy.org/en/latest/api/gammapy.irf.PSF3D.html (the PSF class that's most important and probably the one to work on first) hasn't been changed to use NDDataArray yet, so doing that change will likely lead to quite a bit of work of fixing up callers and high-level anlaysis code, i.e. something best done separate from a PR with the main focus on FITS I/O.

thomasarmstrong added some commits Nov 20, 2017

Applying requested changes to IRF write
using astropy Table, OrderedDict along with some other cleaning up
Removed errors in tests
Removed errors in the test functions, mainly due to the fact that the exposed axis properties were removed.
Corrected column names
Correted instances where ENERGY_X was used over ENERG_X, cases where OFFSET was used over THETA and in energy dispersion used ETRUE? Not sure over this one though as documentation ob IRF formats seems to suggest it should be named ENERG (https://gamma-astro-data-formats.readthedocs.io/en/latest/irfs/full_enclosure/edisp/index.html)
Corrected colum names
Missed some in the tests (didn't get caught on  my machine as I didn't have the gammapy-extras path set)

@cdeil cdeil changed the title from Implement irf write to Add IRF write methods Nov 21, 2017

@cdeil

@thomasarmstrong - I've left one last round of inline comments. It's quite a few changes, but I hope they should all relatively straightforward to address. Let me know if you have any questions or disagree with something!

Finally, there should be at least a 1-line docstring for the to_table methods, like

def to_table(self):
    """Convert to `~astropy.table.Table`."""

And how about adding one-line methods like this?

def to_fits(self):
   """Convert to `~astropy.io.fits.BinTable`."""
    return table_to_fits_table(self.to_table())

I know its a bit of boilerplate, but it's convenient to have from calling code doing FITS I/O like in your example_write_irf.py, while still letting us keep the "clean" serialisation format to_table and from_table which could be the basis for other serialisation formats like already directly to HDF5, which Astropy Table supports.

Show outdated Hide outdated gammapy/irf/energy_dispersion.py Outdated
Show outdated Hide outdated gammapy/irf/background.py Outdated
Show outdated Hide outdated gammapy/irf/background.py Outdated
Show outdated Hide outdated gammapy/irf/background.py Outdated
Show outdated Hide outdated gammapy/irf/effective_area.py Outdated

@cdeil cdeil modified the milestones: 0.8, 0.7 Nov 21, 2017

@ZiweiOu

This comment has been minimized.

Show comment
Hide comment
@ZiweiOu

ZiweiOu Nov 21, 2017

ZiweiOu commented Nov 21, 2017

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Nov 21, 2017

Member

@ZiweiOu - Please call me Christoph (or @cdeil on Github). Also - please always start a new thread on the Gammapy mailing list (https://groups.google.com/forum/#!forum/gammapy) or the Gammapy issue tracker (https://github.com/gammapy/gammapy/issues/new), unless a given thread or issue is exactly on point or very much related to your issue.

In this case, it's not related to the work that @thomasarmstrong is doing here, so I've gone ahead and filed a separate issue #1216 concerning PSF kernels. @ZiweiOu - You can subscribe to that issue if you want to follow the discussion on the PSF kernel (see https://help.github.com/articles/subscribing-to-and-unsubscribing-from-notifications/#subscribing-to-issues-and-pull-requests).

Member

cdeil commented Nov 21, 2017

@ZiweiOu - Please call me Christoph (or @cdeil on Github). Also - please always start a new thread on the Gammapy mailing list (https://groups.google.com/forum/#!forum/gammapy) or the Gammapy issue tracker (https://github.com/gammapy/gammapy/issues/new), unless a given thread or issue is exactly on point or very much related to your issue.

In this case, it's not related to the work that @thomasarmstrong is doing here, so I've gone ahead and filed a separate issue #1216 concerning PSF kernels. @ZiweiOu - You can subscribe to that issue if you want to follow the discussion on the PSF kernel (see https://help.github.com/articles/subscribing-to-and-unsubscribing-from-notifications/#subscribing-to-issues-and-pull-requests).

Added requested changes
Also added name field to table_to_fits_table
Show outdated Hide outdated gammapy/irf/psf_king.py Outdated
m_lo = table['MIGRA_LO'].quantity[0]
m_hi = table['MIGRA_HI'].quantity[0]
matrix = table['MATRIX'].quantity[0].transpose() ## TODO Why does this need to be transposed?

This comment has been minimized.

@cdeil

cdeil Nov 21, 2017

Member

@thomasarmstrong - I think the EnergyDispersion2D class should be changed to remove this transpose in the from_table and to_table, and probably then you also need to update the order of axes as listed in __init__ and then it should work. That would change the numpy array axis order in memory, but code everywhere in Gammapy should evaluate axes by name and all calling code should still work.

That would still be a temp workaround, I think the final solution would be to parse the axis order from the CREF header key. That would make Gammapy independent of the axis order in the FITS file, we could then read any axis order. For generating new FITS files, a class-level attribute that specifies the "preferred" or "default" axis order would still be needed, i.e. for code that creates IRFs from scratch like you want to in ctapipe.

Previous discussion on IRF axis order is at open-gamma-ray-astro/gamma-astro-data-formats#28 and probably other issues.

My suggestion would be that you try the small change I mentioned and see if python setup.py test -V still passes, but leave the larger change involving CREF to a future PR. (or directly postpone this to a future PR).

@cdeil

cdeil Nov 21, 2017

Member

@thomasarmstrong - I think the EnergyDispersion2D class should be changed to remove this transpose in the from_table and to_table, and probably then you also need to update the order of axes as listed in __init__ and then it should work. That would change the numpy array axis order in memory, but code everywhere in Gammapy should evaluate axes by name and all calling code should still work.

That would still be a temp workaround, I think the final solution would be to parse the axis order from the CREF header key. That would make Gammapy independent of the axis order in the FITS file, we could then read any axis order. For generating new FITS files, a class-level attribute that specifies the "preferred" or "default" axis order would still be needed, i.e. for code that creates IRFs from scratch like you want to in ctapipe.

Previous discussion on IRF axis order is at open-gamma-ray-astro/gamma-astro-data-formats#28 and probably other issues.

My suggestion would be that you try the small change I mentioned and see if python setup.py test -V still passes, but leave the larger change involving CREF to a future PR. (or directly postpone this to a future PR).

Applied requested changes
Cleaned up Quantity calls
removed transposing of data in effectivearea2D and energydispersion2D
Incuded fits read/write tests
@thomasarmstrong

This comment has been minimized.

Show comment
Hide comment
@thomasarmstrong

thomasarmstrong Nov 23, 2017

Contributor

There seems to be some problems elsewhere in gammapy from changing the data axis, I will work on this and add a commit shortly.

Contributor

thomasarmstrong commented Nov 23, 2017

There seems to be some problems elsewhere in gammapy from changing the data axis, I will work on this and add a commit shortly.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Nov 23, 2017

Member

@thomasarmstrong - Thanks! If it's confusing, I can also take a look and fix up other parts of the Gammapy code or tests by adding a commit here. Let me know.

Member

cdeil commented Nov 23, 2017

@thomasarmstrong - Thanks! If it's confusing, I can also take a look and fix up other parts of the Gammapy code or tests by adding a commit here. Let me know.

@thomasarmstrong

This comment has been minimized.

Show comment
Hide comment
@thomasarmstrong

thomasarmstrong Nov 24, 2017

Contributor

@cdeil - I am starting to wonder if it might be easier to leave the transpose of the effective area and energy dispersion data arrays... There are many places throughout the code that call for the data/axis to be in the original format, which seems like a lot of work to do for little gain? I have little opinion on what is better so I defer to your insight.

Contributor

thomasarmstrong commented Nov 24, 2017

@cdeil - I am starting to wonder if it might be easier to leave the transpose of the effective area and energy dispersion data arrays... There are many places throughout the code that call for the data/axis to be in the original format, which seems like a lot of work to do for little gain? I have little opinion on what is better so I defer to your insight.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Nov 24, 2017

Member

@thomasarmstrong - I would suggest to defer the EDISP transpose change to later.

Otherwise - is this PR ready from your side?

Member

cdeil commented Nov 24, 2017

@thomasarmstrong - I would suggest to defer the EDISP transpose change to later.

Otherwise - is this PR ready from your side?

Reverted to transposed edisp and aeff data
Returned the axis order to EnergyDispersion2D and EffectiveAreaTable2D for now, as there are other functions that require this order.
@thomasarmstrong

This comment has been minimized.

Show comment
Hide comment
@thomasarmstrong

thomasarmstrong Nov 24, 2017

Contributor

@cdeil - I have reverted some of the changes and it should be ready now. I guess the next step is to open a new issue/PR for the PSF classes to use NDDataArray and to be compatible with the current CTA IRFs?

Contributor

thomasarmstrong commented Nov 24, 2017

@cdeil - I have reverted some of the changes and it should be ready now. I guess the next step is to open a new issue/PR for the PSF classes to use NDDataArray and to be compatible with the current CTA IRFs?

@cdeil

cdeil approved these changes Nov 24, 2017

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Nov 24, 2017

Member

@thomasarmstrong - Thanks! I don't have time today, but I will merge this in tomorrow.

I think the next steps are:

  • Improve code and tests for gammapy.irf.PSF3D, especially use NDDataArray to store the IRF and make FITS I/O work.
  • Improve code and tests for gammapy.irf.EnergyDependentMultiGaussPSF. There I think we also want to use NDDataArray, but currently I think we have no case of analytical IRFs using it, and it will need some thought what the interpolation behaviour should be. I think at the moment it is interpolating the model parameters of the analytical model, and that's acceptable behaviour and should be possible to go to NDDataArray and FITS I/O like for the other IRF classes.
  • I also think a reminder issue that the data array transpose on IRF FITS read / write should be looked at and probably best to clean up Gammapy code to avoid it. (not sure, needs some thought / discussion)
  • And more generally we might or not might want to change NDDataArray to support arbitrary axis orders, and in FITS I/O use the CREF header key to determine axis order. (not sure, needs some thought / discussion)
  • We probably also want some code to smooth and fits IRF histograms to derive smooth or anlytical IRFs. For that I'm not sure where it should live. It could partly be methods on the IRF classes, or separate functions or classes in gammapy.irf or in ctapipe.
  • Finally, we have several old very preliminary formats and classes for IRF code, that comes from first attempts to export to FITS in HESS from a few years ago. There I have the overview and will clean that up, i.e. mostly remove them, in the coming weeks.

Of course pull requests (probably one per class is best, keep them small) are welcome any time, but it's also clear that this is a big task for the coming months and we should try to discuss and distribute the work a bit. From the Gammapy side, we now have bi-weekly calls on eZuce on Fridays at 11 am, the next ones are Dec 1 and 15. We are also starting to plan our next coding sprint, which will be early next year in Paris. If you're interested, please fill https://doodle.com/poll/2q8rw8n8edpm6fn5 and for the weekly calls, let @bkhelifi or me know if you have time to join and talk about IRFs.

Member

cdeil commented Nov 24, 2017

@thomasarmstrong - Thanks! I don't have time today, but I will merge this in tomorrow.

I think the next steps are:

  • Improve code and tests for gammapy.irf.PSF3D, especially use NDDataArray to store the IRF and make FITS I/O work.
  • Improve code and tests for gammapy.irf.EnergyDependentMultiGaussPSF. There I think we also want to use NDDataArray, but currently I think we have no case of analytical IRFs using it, and it will need some thought what the interpolation behaviour should be. I think at the moment it is interpolating the model parameters of the analytical model, and that's acceptable behaviour and should be possible to go to NDDataArray and FITS I/O like for the other IRF classes.
  • I also think a reminder issue that the data array transpose on IRF FITS read / write should be looked at and probably best to clean up Gammapy code to avoid it. (not sure, needs some thought / discussion)
  • And more generally we might or not might want to change NDDataArray to support arbitrary axis orders, and in FITS I/O use the CREF header key to determine axis order. (not sure, needs some thought / discussion)
  • We probably also want some code to smooth and fits IRF histograms to derive smooth or anlytical IRFs. For that I'm not sure where it should live. It could partly be methods on the IRF classes, or separate functions or classes in gammapy.irf or in ctapipe.
  • Finally, we have several old very preliminary formats and classes for IRF code, that comes from first attempts to export to FITS in HESS from a few years ago. There I have the overview and will clean that up, i.e. mostly remove them, in the coming weeks.

Of course pull requests (probably one per class is best, keep them small) are welcome any time, but it's also clear that this is a big task for the coming months and we should try to discuss and distribute the work a bit. From the Gammapy side, we now have bi-weekly calls on eZuce on Fridays at 11 am, the next ones are Dec 1 and 15. We are also starting to plan our next coding sprint, which will be early next year in Paris. If you're interested, please fill https://doodle.com/poll/2q8rw8n8edpm6fn5 and for the weekly calls, let @bkhelifi or me know if you have time to join and talk about IRFs.

@thomasarmstrong thomasarmstrong merged commit 9fb2508 into gammapy:master Nov 25, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@thomasarmstrong thomasarmstrong deleted the thomasarmstrong:implement_irf_write branch Nov 25, 2017

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