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 OffDataBackgroundMaker #485

Merged
merged 15 commits into from Mar 14, 2016

Conversation

Projects
None yet
2 participants
@JouvinLea
Contributor

JouvinLea commented Mar 7, 2016

PR for the script of how to use the new class.
Tell me what you think! It's quite the same than the test_background_model!

Lea Jouvin
example of the use of the new class
Please enter the commit message for your changes. Lines starting
@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Mar 7, 2016

Member

If I had started the examples/test_backgroundmodel.py, it would probably have been this (code not tested, just typed):

from gammapy.data import DataStore
from gammapy.background import OffDataBackgroundMaker

def test_background_model():
    data_store = DataStore.from_dir('$GAMMAPY_EXTRA/datasets/hess-crab4-hd-hap-prod2/')
    out_dir = # somewhere where all temp / output files will go
    bgmaker = OffDataBackgroundMaker(data_store, out_dir)
    bgmaker.select_observations( ... options ... )
    bgmaker.group_observations( ... options ... )
    bgmaker.make_model(... options ... )
    bgmaker.save()

if __name__ == '__main__':
    test_background_model()

And then I'd comment in / out the method calls for the steps I want to test.
It's very similar to your test script, except it's shorter.

I'm not sure about how exactly the class should work. There's many detailed decisions to make, e.g. when to store things on disk or only as members on the bgmaker class. Or which options to take in the __init__ and which in later methods ...

Actually it could be useful to have two or three such small test_background_model functions that represent different use cases such as 2D or 3D model, or the case where the obs list is computed via a method or given as input from the outside by the caller (e.g. by bgmaker.obs_table = my_obs_table or giving it in the __init__ already.

But that can also be done later ... the most important thing is that you get this example working with the new class (and get the same output):
https://github.com/gammapy/gammapy/blob/master/gammapy/scripts/tests/test_background_model.py#L22

OK?

Member

cdeil commented Mar 7, 2016

If I had started the examples/test_backgroundmodel.py, it would probably have been this (code not tested, just typed):

from gammapy.data import DataStore
from gammapy.background import OffDataBackgroundMaker

def test_background_model():
    data_store = DataStore.from_dir('$GAMMAPY_EXTRA/datasets/hess-crab4-hd-hap-prod2/')
    out_dir = # somewhere where all temp / output files will go
    bgmaker = OffDataBackgroundMaker(data_store, out_dir)
    bgmaker.select_observations( ... options ... )
    bgmaker.group_observations( ... options ... )
    bgmaker.make_model(... options ... )
    bgmaker.save()

if __name__ == '__main__':
    test_background_model()

And then I'd comment in / out the method calls for the steps I want to test.
It's very similar to your test script, except it's shorter.

I'm not sure about how exactly the class should work. There's many detailed decisions to make, e.g. when to store things on disk or only as members on the bgmaker class. Or which options to take in the __init__ and which in later methods ...

Actually it could be useful to have two or three such small test_background_model functions that represent different use cases such as 2D or 3D model, or the case where the obs list is computed via a method or given as input from the outside by the caller (e.g. by bgmaker.obs_table = my_obs_table or giving it in the __init__ already.

But that can also be done later ... the most important thing is that you get this example working with the new class (and get the same output):
https://github.com/gammapy/gammapy/blob/master/gammapy/scripts/tests/test_background_model.py#L22

OK?

@cdeil cdeil added this to the 0.4 milestone Mar 7, 2016

@cdeil cdeil self-assigned this Mar 7, 2016

@JouvinLea

This comment has been minimized.

Show comment
Hide comment
@JouvinLea

JouvinLea Mar 7, 2016

Contributor

yes I agree with you, I was wondering the same things.
Let have the test works for the class as it was implement in background_model and then we will see how to improve it!

Contributor

JouvinLea commented Mar 7, 2016

yes I agree with you, I was wondering the same things.
Let have the test works for the class as it was implement in background_model and then we will see how to improve it!

@JouvinLea

This comment has been minimized.

Show comment
Hide comment
@JouvinLea

JouvinLea Mar 7, 2016

Contributor

By bgmaker.save() you mean to write the rate in a fits file? Because this is already done in make_model(). Perhaps you prefer to put the writing in an other method save() ?

Contributor

JouvinLea commented Mar 7, 2016

By bgmaker.save() you mean to write the rate in a fits file? Because this is already done in make_model(). Perhaps you prefer to put the writing in an other method save() ?

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Mar 7, 2016

Member

Yes, I think in principle it's nicer if the user has control over when / what gets saved to disk.
So there would be no or less saving to disk from the intermediate steps than with the command line, results would get stored as bgmaker data members.

There could also be some more control like:

bgmaker.obs_table.write('... exactly where I like ...')
bgmaker.model.rate.write('... exactly where I like ...')

and most users would use the default:

bgmaker.save(outdir='where I like', debug=True)

which saves the main result or also debug intermediate results (like counts, exposure cubes), to a given output folder.

But if you prefer to handle it some other way, that's fine with me, too.

Member

cdeil commented Mar 7, 2016

Yes, I think in principle it's nicer if the user has control over when / what gets saved to disk.
So there would be no or less saving to disk from the intermediate steps than with the command line, results would get stored as bgmaker data members.

There could also be some more control like:

bgmaker.obs_table.write('... exactly where I like ...')
bgmaker.model.rate.write('... exactly where I like ...')

and most users would use the default:

bgmaker.save(outdir='where I like', debug=True)

which saves the main result or also debug intermediate results (like counts, exposure cubes), to a given output folder.

But if you prefer to handle it some other way, that's fine with me, too.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Mar 7, 2016

Member

You could also have a look at this class as an example, which is of similar purpose (run some algorithm, save some output):
https://gammapy.readthedocs.org/en/latest/api/gammapy.background.IterativeKernelBackgroundEstimator.html

I'm not saying that one is very well done or you should follow the same pattern ... just an example to look at before you implement things here in a way you like.

Member

cdeil commented Mar 7, 2016

You could also have a look at this class as an example, which is of similar purpose (run some algorithm, save some output):
https://gammapy.readthedocs.org/en/latest/api/gammapy.background.IterativeKernelBackgroundEstimator.html

I'm not saying that one is very well done or you should follow the same pattern ... just an example to look at before you implement things here in a way you like.

@JouvinLea

This comment has been minimized.

Show comment
Hide comment
@JouvinLea

JouvinLea Mar 8, 2016

Contributor

Hi,
I implemented the new class with the test that works.
As you said yesterday, I am not sure about the writing in the select_observation() and group_observation() methods?
Maybe we can close this PR is there is no conflict and after your suggestions? We can then improve it if we think this is needed later?

Contributor

JouvinLea commented Mar 8, 2016

Hi,
I implemented the new class with the test that works.
As you said yesterday, I am not sure about the writing in the select_observation() and group_observation() methods?
Maybe we can close this PR is there is no conflict and after your suggestions? We can then improve it if we think this is needed later?

Lea Jouvin
elif modeltype == "2D":
ebounds = EnergyBounds.equal_log_spacing(0.1, 100, 100, 'TeV')
offset = sqrt_space(start=0, stop=2.5, num=100) * u.deg

This comment has been minimized.

@JouvinLea

JouvinLea Mar 8, 2016

Contributor

I changed to a sqrt distribution.
Maybe we have to give the energy and offset as parameters no? This is weird to define them here?

@JouvinLea

JouvinLea Mar 8, 2016

Contributor

I changed to a sqrt distribution.
Maybe we have to give the energy and offset as parameters no? This is weird to define them here?

Lea Jouvin added some commits Mar 9, 2016

Lea Jouvin
Lea Jouvin
Lea Jouvin
Show outdated Hide outdated gammapy/background/__init__.py Outdated
Show outdated Hide outdated gammapy/data/data_store.py Outdated

@cdeil cdeil changed the title from Class OffDataBackgroundMaker to Add OffDataBackgroundMaker Mar 11, 2016

Lea Jouvin added some commits Mar 11, 2016

Lea Jouvin added some commits Mar 11, 2016

Lea Jouvin
self.outdir = "out"
else:
self.outdir = outdir
self.obs_table = obs_table

This comment has been minimized.

@JouvinLea

JouvinLea Mar 11, 2016

Contributor

I added this two parameters as class members, was it what you think about?

@JouvinLea

JouvinLea Mar 11, 2016

Contributor

I added this two parameters as class members, was it what you think about?

cdeil added a commit that referenced this pull request Mar 14, 2016

@cdeil cdeil merged commit 9aba582 into gammapy:master Mar 14, 2016

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment