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

Make background cube models #319

Merged
merged 50 commits into from Aug 21, 2015

Conversation

Projects
None yet
2 participants
@mapazarr
Member

mapazarr commented Aug 8, 2015

Adding script to produce bg cube models. This is a continuation of the work started in: gammasky/hess-host-analyses#1.
TODO list:

  • divide the code into smaller functions
  • move most of the functionality to Gammapy classes/functions
  • implement H.E.S.S. to Gammapy obs lists converter
  • reorganize the bg models class
  • add simple dummy bg event list generator
  • add tests for scripts and new methods in Gammapy
  • add script to compare 2 sets of bg cube models
  • remove DEBUG plots
  • implement comments from: gammasky/hess-host-analyses#1 (and this PR)
  • update/implement docs (stringdocs and high-level docs)

@mapazarr mapazarr added this to the 0.4 milestone Aug 8, 2015

@mapazarr

This comment has been minimized.

Show comment
Hide comment
@mapazarr

mapazarr Aug 8, 2015

Member

I'm aware of the travis-ci failure. It's because of the use of matplotlib without marking it as optional dependency (using the proper decorator in the tests). I'll work on that. It'll get fixed once I remove the plotting part (used for DEBUG for now).

Member

mapazarr commented Aug 8, 2015

I'm aware of the travis-ci failure. It's because of the use of matplotlib without marking it as optional dependency (using the proper decorator in the tests). I'll work on that. It'll get fixed once I remove the plotting part (used for DEBUG for now).

@mapazarr

This comment has been minimized.

Show comment
Hide comment
@mapazarr

mapazarr Aug 8, 2015

Member

@cdeil I will need some fake data (event lists) for the tests. Is there something available? Otherwise I would write a supersimple random tool to do so.

Member

mapazarr commented Aug 8, 2015

@cdeil I will need some fake data (event lists) for the tests. Is there something available? Otherwise I would write a supersimple random tool to do so.

@mapazarr

This comment has been minimized.

Show comment
Hide comment
@mapazarr

mapazarr Aug 8, 2015

Member

I'm aware of the travis-ci failure. It's because of the use of scipy without marking it as optional dependency (using the proper decorator in the tests). I'll work on that.

Member

mapazarr commented Aug 8, 2015

I'm aware of the travis-ci failure. It's because of the use of scipy without marking it as optional dependency (using the proper decorator in the tests). I'll work on that.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Aug 9, 2015

Member

@mapazarr – A simple tool that samples background events according to some simple shape and spectrum would be good, even if it takes a day (but not more) to build. This way you'll have an easy way to generate example data, and you'll have "true" background cube models that you can compare the "reconstructed" ones against.

Member

cdeil commented Aug 9, 2015

@mapazarr – A simple tool that samples background events according to some simple shape and spectrum would be good, even if it takes a day (but not more) to build. This way you'll have an easy way to generate example data, and you'll have "true" background cube models that you can compare the "reconstructed" ones against.

@cdeil cdeil added the feature label Aug 10, 2015

@cdeil cdeil self-assigned this Aug 10, 2015

@mapazarr

This comment has been minimized.

Show comment
Hide comment
@mapazarr

mapazarr Aug 10, 2015

Member

I'm aware that I need to rebase. Since the PR is still WIP, I'll do this later.

Member

mapazarr commented Aug 10, 2015

I'm aware that I need to rebase. Since the PR is still WIP, I'll do this later.

@mapazarr

This comment has been minimized.

Show comment
Hide comment
@mapazarr

mapazarr Aug 11, 2015

Member

@cdeil I have divided the bg cube model production into smaller functions and added them to Gammapy. (I still need to do it for the observation grouping.) Can you please have a look at the structure of the code in background/make.py and tell me if you like it?

Member

mapazarr commented Aug 11, 2015

@cdeil I have divided the bg cube model production into smaller functions and added them to Gammapy. (I still need to do it for the observation grouping.) Can you please have a look at the structure of the code in background/make.py and tell me if you like it?

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

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Aug 11, 2015

Member

Please rebase on master now (making a backup copy before you start).
This has several advantages:

  • travis-ci runs
  • I can more easily try out your branch
  • sometimes the diff on github is more accurate
  • one large rebase at the end is harder than smaller rebases with only few conflicts from time to time.
Member

cdeil commented Aug 11, 2015

Please rebase on master now (making a backup copy before you start).
This has several advantages:

  • travis-ci runs
  • I can more easily try out your branch
  • sometimes the diff on github is more accurate
  • one large rebase at the end is harder than smaller rebases with only few conflicts from time to time.
Show outdated Hide outdated gammapy/background/make.py Outdated
Show outdated Hide outdated gammapy/background/make.py Outdated
Show outdated Hide outdated gammapy/background/make.py Outdated
Show outdated Hide outdated gammapy/background/make.py Outdated
Show outdated Hide outdated gammapy/background/make.py Outdated
Show outdated Hide outdated gammapy/background/make.py Outdated
Show outdated Hide outdated setup.py Outdated
@@ -10,3 +10,4 @@
from .template import *
from .kernel import *
from .models import *
from .make import *

This comment has been minimized.

@cdeil

cdeil Aug 11, 2015

Member

I think make is too generic a name, plus it would be good to have the class and functions operating on that class (if any remain standalone) in the same file.
So how about renaming make.py -> cube_model.py and moving the CubeBackgroundModel class here?

@cdeil

cdeil Aug 11, 2015

Member

I think make is too generic a name, plus it would be good to have the class and functions operating on that class (if any remain standalone) in the same file.
So how about renaming make.py -> cube_model.py and moving the CubeBackgroundModel class here?

This comment has been minimized.

@mapazarr

mapazarr Aug 12, 2015

Member

Well, most of the functions should probably move to other places. I like having the generic naming of models.py and make.py, in case other models come in the future and can share the same files. I don't have a strong opinion on that. I was just following the same convention as in the dataset module.

After this clarification: should I keep it as it is? or should I use cube specific naming?

BTW, the generic Cube class mentioned in #319 (diff) could go into a cube.py file, similar to maps.py.

@mapazarr

mapazarr Aug 12, 2015

Member

Well, most of the functions should probably move to other places. I like having the generic naming of models.py and make.py, in case other models come in the future and can share the same files. I don't have a strong opinion on that. I was just following the same convention as in the dataset module.

After this clarification: should I keep it as it is? or should I use cube specific naming?

BTW, the generic Cube class mentioned in #319 (diff) could go into a cube.py file, similar to maps.py.

This comment has been minimized.

@cdeil

cdeil Aug 12, 2015

Member

I especially don't like make.py, because a year from now there will be lots of other code that makes things in gammpay.background, e.g. reflected regions or exclusion regions or ... and that code shouldn't be in the same file as what's in make.py now.

So either cube_model.py like I suggested, or cube.py like you did seems better to me.

But in the end ... we will accumulate functionality for a while and then try to do a consistent naming across Gammapy before the 1.0 release. So it's not terribly important what we choose now ... you decide!

@cdeil

cdeil Aug 12, 2015

Member

I especially don't like make.py, because a year from now there will be lots of other code that makes things in gammpay.background, e.g. reflected regions or exclusion regions or ... and that code shouldn't be in the same file as what's in make.py now.

So either cube_model.py like I suggested, or cube.py like you did seems better to me.

But in the end ... we will accumulate functionality for a while and then try to do a consistent naming across Gammapy before the 1.0 release. So it's not terribly important what we choose now ... you decide!

This comment has been minimized.

@mapazarr

mapazarr Aug 21, 2015

Member

Done. I moved all methods to either Cube in cube.py or CubeBackgroundModel in models.py. I left only the skeleton in make_bg_cube_model in make.py.

@mapazarr

mapazarr Aug 21, 2015

Member

Done. I moved all methods to either Cube in cube.py or CubeBackgroundModel in models.py. I left only the skeleton in make_bg_cube_model in make.py.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Aug 11, 2015

Member

@mapazarr – I've left a bunch of inline comments. Some (like rebase, logging) are straightforward to address, others (like grouping, configuring) are vague and need more thought / discussion.

One thing that would help me very much to give useful feedback on this would be if I had a high-level docs page with examples how to run the command line tool so that I can quickly start playing around with this. I'm not sure if generating a ~ 10 run example dataset and using it for a public docs page in Gammapy is best, or a HESS-internal page showing how to do it for a small and quick and the full HESS off run dataset. Ideally both. :-)

Member

cdeil commented Aug 11, 2015

@mapazarr – I've left a bunch of inline comments. Some (like rebase, logging) are straightforward to address, others (like grouping, configuring) are vague and need more thought / discussion.

One thing that would help me very much to give useful feedback on this would be if I had a high-level docs page with examples how to run the command line tool so that I can quickly start playing around with this. I'm not sure if generating a ~ 10 run example dataset and using it for a public docs page in Gammapy is best, or a HESS-internal page showing how to do it for a small and quick and the full HESS off run dataset. Ideally both. :-)

@mapazarr

This comment has been minimized.

Show comment
Hide comment
@mapazarr

mapazarr Aug 12, 2015

Member

@cdeil OK, I added a documentation page. Sorry, I thought it was clear (for now), when running the inline command with --help and left the doc for later. It's a subpage of the main background page.

I still didn't have time to work on the test data for Gammapy. As for now, it runs on H.E.S.S. data (prod02). You can run a quick test with:

gammapy-make_bg_cube_models /path/to/fits/event_lists/base/dir --test True

The whole production (without --test) takes ~ 5 min on my computer.

Member

mapazarr commented Aug 12, 2015

@cdeil OK, I added a documentation page. Sorry, I thought it was clear (for now), when running the inline command with --help and left the doc for later. It's a subpage of the main background page.

I still didn't have time to work on the test data for Gammapy. As for now, it runs on H.E.S.S. data (prod02). You can run a quick test with:

gammapy-make_bg_cube_models /path/to/fits/event_lists/base/dir --test True

The whole production (without --test) takes ~ 5 min on my computer.

Show outdated Hide outdated docs/background/make_models.rst Outdated
Show outdated Hide outdated docs/background/make_models.rst Outdated
@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Aug 12, 2015

Member

I'm seeing this error from ftverify for the cube model image FITS header:
https://gist.github.com/cdeil/8665a22897c6ebe71680#file-gistfile1-txt-L1
If it's not clear how to fix it, please put it on the agenda for our hangout tonight.

Member

cdeil commented Aug 12, 2015

I'm seeing this error from ftverify for the cube model image FITS header:
https://gist.github.com/cdeil/8665a22897c6ebe71680#file-gistfile1-txt-L1
If it's not clear how to fix it, please put it on the agenda for our hangout tonight.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Aug 12, 2015

Member

I ran the test example and it worked. 👏

log and generated files:

$ find .
.
./bg_cube_models
./bg_cube_models/bg_cube_model_alt0_az0_image.fits.gz
./bg_cube_models/bg_cube_model_alt0_az0_table.fits.gz
./bg_cube_models/bg_cube_model_alt1_az0_image.fits.gz
./bg_cube_models/bg_cube_model_alt1_az0_table.fits.gz
./bg_observation_table.fits.gz
./splitted_obs_list
./splitted_obs_list/bg_observation_table_alt0_az0.fits.gz
./splitted_obs_list/bg_observation_table_alt1_az0.fits.gz

A few suggestions for improvement based on this first impression:

  • Don't be so chatty in the log (e.g. all the obs tables shouldn't be printed, or if you really want to, make this debug output.
  • As already mentioned, use group_XXXX with the GROUP_ID instead of alt0_az0 (scales to other grouping schemes).
  • Probably best to put the group obs tables in the same folder (or even file as an extra extension) as the cubes.
  • We need a way to store the obs grouping info on disk, so that it can be later used to associate observations with background models, like @mimayer when he creates a soft link for each obs to a background model. This is probably for a different script and PR, but this script has to store the required obs grouping info somehow. This is difficult and we can try to find a solution in the hangout tonight.
Member

cdeil commented Aug 12, 2015

I ran the test example and it worked. 👏

log and generated files:

$ find .
.
./bg_cube_models
./bg_cube_models/bg_cube_model_alt0_az0_image.fits.gz
./bg_cube_models/bg_cube_model_alt0_az0_table.fits.gz
./bg_cube_models/bg_cube_model_alt1_az0_image.fits.gz
./bg_cube_models/bg_cube_model_alt1_az0_table.fits.gz
./bg_observation_table.fits.gz
./splitted_obs_list
./splitted_obs_list/bg_observation_table_alt0_az0.fits.gz
./splitted_obs_list/bg_observation_table_alt1_az0.fits.gz

A few suggestions for improvement based on this first impression:

  • Don't be so chatty in the log (e.g. all the obs tables shouldn't be printed, or if you really want to, make this debug output.
  • As already mentioned, use group_XXXX with the GROUP_ID instead of alt0_az0 (scales to other grouping schemes).
  • Probably best to put the group obs tables in the same folder (or even file as an extra extension) as the cubes.
  • We need a way to store the obs grouping info on disk, so that it can be later used to associate observations with background models, like @mimayer when he creates a soft link for each obs to a background model. This is probably for a different script and PR, but this script has to store the required obs grouping info somehow. This is difficult and we can try to find a solution in the hangout tonight.
@mapazarr

This comment has been minimized.

Show comment
Hide comment
@mapazarr

mapazarr Aug 12, 2015

Member

@deil I'm aware of the problematic related to the observation grouping. I'm working on an observation groups class that stores the groups into an ~astropy.table.Table. This can easily be saved to disk and read back.

Here a screenshot of an example observation groups table

obs_groups_test

Member

mapazarr commented Aug 12, 2015

@deil I'm aware of the problematic related to the observation grouping. I'm working on an observation groups class that stores the groups into an ~astropy.table.Table. This can easily be saved to disk and read back.

Here a screenshot of an example observation groups table

obs_groups_test

@mapazarr mapazarr referenced this pull request Aug 21, 2015

Closed

Improve cube bg model production #333

1 of 20 tasks complete
@mapazarr

This comment has been minimized.

Show comment
Hide comment
@mapazarr

mapazarr Aug 21, 2015

Member

@cdeil OK, I addressed almost all comments. Please don't add any new ones, since I still need to finish a couple of thing (and sleep).

Member

mapazarr commented Aug 21, 2015

@cdeil OK, I addressed almost all comments. Please don't add any new ones, since I still need to finish a couple of thing (and sleep).

mapazarr added a commit to mapazarr/gammapy that referenced this pull request Aug 21, 2015

@mapazarr

This comment has been minimized.

Show comment
Hide comment
@mapazarr

mapazarr Aug 21, 2015

Member

@deil Ok, I finished with the comments. Can you please press the green button? There is surely room for improvement, but I can't stay awake longer.

Member

mapazarr commented Aug 21, 2015

@deil Ok, I finished with the comments. Can you please press the green button? There is surely room for improvement, but I can't stay awake longer.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Aug 21, 2015

Member

@mapazarr – Merging this now. Thank you!

This is the biggest PR we've ever had in Gammapy (+3,682 −882). It contains a lot of great new functionality (observation handling, grouping, cube background model production).

One issue is that this goes in without substantial code review of methods, API, implementation, tests, docs because GSoC is over and I wasn't available much.
So we'll have to see how we handle this for the coming weeks / months. One possibility is that I take some time (a few days needed?) review the diff of this PR and the task list in the follow-up issue #333 to this PR.
Or we'll just do a big systematic review of all of Gammapy and try to improve it in the months before the 1.0 release ... probably we should do this as most code in Gammapy hasn't been thoroughly reviewed and many things changed (like became available in Astropy or we decided to use Sherpa).
Or a little bit of both.

Member

cdeil commented Aug 21, 2015

@mapazarr – Merging this now. Thank you!

This is the biggest PR we've ever had in Gammapy (+3,682 −882). It contains a lot of great new functionality (observation handling, grouping, cube background model production).

One issue is that this goes in without substantial code review of methods, API, implementation, tests, docs because GSoC is over and I wasn't available much.
So we'll have to see how we handle this for the coming weeks / months. One possibility is that I take some time (a few days needed?) review the diff of this PR and the task list in the follow-up issue #333 to this PR.
Or we'll just do a big systematic review of all of Gammapy and try to improve it in the months before the 1.0 release ... probably we should do this as most code in Gammapy hasn't been thoroughly reviewed and many things changed (like became available in Astropy or we decided to use Sherpa).
Or a little bit of both.

cdeil added a commit that referenced this pull request Aug 21, 2015

@cdeil cdeil merged commit 912db36 into gammapy:master Aug 21, 2015

1 check passed

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

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Aug 21, 2015

Member

The Sphinx build on RTD succeeded:
https://readthedocs.org/projects/gammapy/builds/3264248/
(and RTD since yesterday? shows a much better build log, with the commands they are running and the corresponding output)

Here's some of the new high-level docs pages from this PR ... they all look OK to me (formatting-wise ... I have to take a day next week to go through them):

Member

cdeil commented Aug 21, 2015

The Sphinx build on RTD succeeded:
https://readthedocs.org/projects/gammapy/builds/3264248/
(and RTD since yesterday? shows a much better build log, with the commands they are running and the corresponding output)

Here's some of the new high-level docs pages from this PR ... they all look OK to me (formatting-wise ... I have to take a day next week to go through them):

@mapazarr

This comment has been minimized.

Show comment
Hide comment
@mapazarr

mapazarr Aug 21, 2015

Member

snapshot42
Release Gammapy 0.3.1 ? :-D

Member

mapazarr commented Aug 21, 2015

snapshot42
Release Gammapy 0.3.1 ? :-D

@mapazarr

This comment has been minimized.

Show comment
Hide comment
@mapazarr

mapazarr Aug 21, 2015

Member

The work should continue in #333, or other subsequent PRs.

Member

mapazarr commented Aug 21, 2015

The work should continue in #333, or other subsequent PRs.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Aug 21, 2015

Member

Release Gammapy 0.3.1?

No, we'll just release 0.4 in ~ 1 month.
Better to get some usage / testing for the stuff from this PR. And I also want to have a first version of morphology and spectrum fitting in the next release (which is what most users want).

Member

cdeil commented Aug 21, 2015

Release Gammapy 0.3.1?

No, we'll just release 0.4 in ~ 1 month.
Better to get some usage / testing for the stuff from this PR. And I also want to have a first version of morphology and spectrum fitting in the next release (which is what most users want).

JonathanDHarris added a commit to JonathanDHarris/gammapy that referenced this pull request Sep 25, 2015

JonathanDHarris added a commit to JonathanDHarris/gammapy that referenced this pull request Sep 25, 2015

JonathanDHarris added a commit to JonathanDHarris/gammapy that referenced this pull request Sep 25, 2015

JonathanDHarris added a commit to JonathanDHarris/gammapy that referenced this pull request Sep 25, 2015

JonathanDHarris added a commit to JonathanDHarris/gammapy that referenced this pull request Sep 25, 2015

JonathanDHarris added a commit to JonathanDHarris/gammapy that referenced this pull request Sep 25, 2015

JonathanDHarris added a commit to JonathanDHarris/gammapy that referenced this pull request Sep 25, 2015

dlennarz pushed a commit to dlennarz/gammapy that referenced this pull request Oct 11, 2015

dlennarz pushed a commit to dlennarz/gammapy that referenced this pull request Oct 11, 2015

dlennarz pushed a commit to dlennarz/gammapy that referenced this pull request Oct 11, 2015

dlennarz pushed a commit to dlennarz/gammapy that referenced this pull request Oct 11, 2015

dlennarz pushed a commit to dlennarz/gammapy that referenced this pull request Oct 11, 2015

dlennarz pushed a commit to dlennarz/gammapy that referenced this pull request Oct 11, 2015

dlennarz pushed a commit to dlennarz/gammapy that referenced this pull request Oct 11, 2015

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