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 SpectrumAnalysisIACT #1001

Merged
merged 2 commits into from Apr 27, 2017

Conversation

Projects
None yet
2 participants
@joleroi
Contributor

joleroi commented Apr 27, 2017

@joleroi joleroi added the feature label Apr 27, 2017

@joleroi joleroi added this to the 0.6 milestone Apr 27, 2017

@joleroi joleroi self-assigned this Apr 27, 2017

@joleroi joleroi requested a review from cdeil Apr 27, 2017

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Apr 27, 2017

Member

Maybe the example in the "Getting started" should be changed to use this?
It's very rare that a user has PHA files already, no?
https://gammapy.readthedocs.io/en/latest/spectrum/index.html

So config is a dict that has a mix of Python (number, string) and Gammapy objects?
I'm not sure if it's a good idea (can't be read / written from config file), but let's try it out for a while and see if we like it.
But to be usable you have to give a list of keys in the config dict that the algorithm understands in the class docstring, no?

Member

cdeil commented Apr 27, 2017

Maybe the example in the "Getting started" should be changed to use this?
It's very rare that a user has PHA files already, no?
https://gammapy.readthedocs.io/en/latest/spectrum/index.html

So config is a dict that has a mix of Python (number, string) and Gammapy objects?
I'm not sure if it's a good idea (can't be read / written from config file), but let's try it out for a while and see if we like it.
But to be usable you have to give a list of keys in the config dict that the algorithm understands in the class docstring, no?

@joleroi

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Apr 27, 2017

Contributor

So config is a dict that has a mix of Python (number, string) and Gammapy objects?
I'm not sure if it's a good idea (can't be read / written from config file), but let's try it out for a while and see if we like it.

Yes, we have to find a general way to serialize e.g. astropy.regions or even Quantities. This should not be done in this class, so I leave it open for later

But to be usable you have to give a list of keys in the config dict that the algorithm understands in the class docstring, no?

Very complex, don't you think the usage example is enough?

Contributor

joleroi commented Apr 27, 2017

So config is a dict that has a mix of Python (number, string) and Gammapy objects?
I'm not sure if it's a good idea (can't be read / written from config file), but let's try it out for a while and see if we like it.

Yes, we have to find a general way to serialize e.g. astropy.regions or even Quantities. This should not be done in this class, so I leave it open for later

But to be usable you have to give a list of keys in the config dict that the algorithm understands in the class docstring, no?

Very complex, don't you think the usage example is enough?

@cdeil

Left a bunch of nitpicky inline comments.

Overall I like this, I think I'll use it all the time.
Let's put this in today, so that people use it and give good feedback from real-world use cases (e.g. running the HESS DR1 analyses).

Show outdated Hide outdated gammapy/scripts/spectrum_pipe.py Outdated
Show outdated Hide outdated gammapy/scripts/spectrum_pipe.py Outdated
# TODO: Don't stack again if SpectrumFit has already done the stacking
stacked_obs = self.extraction.observations.stack()
self.egm = SpectrumEnergyGroupMaker(stacked_obs)
self.egm.compute_groups_fixed(self.config['fp_binning'])

This comment has been minimized.

@cdeil

cdeil Apr 27, 2017

Member

I would suggest to add a line

groups = self.egm.groups

and then a blank line.
This will IMO make it more clear that there are two steps and how they are (not) coupled.

@cdeil

cdeil Apr 27, 2017

Member

I would suggest to add a line

groups = self.egm.groups

and then a blank line.
This will IMO make it more clear that there are two steps and how they are (not) coupled.

Show outdated Hide outdated gammapy/scripts/spectrum_pipe.py Outdated
Show outdated Hide outdated gammapy/scripts/tests/test_spectrum_pipe.py Outdated
ana.run()
actual = ana.flux_point_estimator.flux_points.table[0]['dnde']
desired = 7.208219787928114e-08
assert_quantity_allclose(actual, desired)

This comment has been minimized.

@cdeil

cdeil Apr 27, 2017

Member

Add a few more asserts on key results?

@cdeil

cdeil Apr 27, 2017

Member

Add a few more asserts on key results?

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Apr 27, 2017

Member

Very complex, don't you think the usage example is enough?

Then users will always have the question "what can / can't this do"?
We have to find a solution for this.

For now, a bullet list like this would be good, no?

Config options:

* spam : int
* ham : dict
    * mam : {'la, 'le', 'lu'}
    * pos : SkyCoord
Member

cdeil commented Apr 27, 2017

Very complex, don't you think the usage example is enough?

Then users will always have the question "what can / can't this do"?
We have to find a solution for this.

For now, a bullet list like this would be good, no?

Config options:

* spam : int
* ham : dict
    * mam : {'la, 'le', 'lu'}
    * pos : SkyCoord
@joleroi

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Apr 27, 2017

Contributor

For now, a bullet list like this would be good, no?

I disagree, this will go out of sync in < 1 day. The config dict is used to instantiate the maker classes (using **). So as soon as one of the maker classes gets a new feature, it would have to be added to your bullet list.

I will add this explanation in the docstring, ok?

* extraction : dict -> passed to XY
Contributor

joleroi commented Apr 27, 2017

For now, a bullet list like this would be good, no?

I disagree, this will go out of sync in < 1 day. The config dict is used to instantiate the maker classes (using **). So as soon as one of the maker classes gets a new feature, it would have to be added to your bullet list.

I will add this explanation in the docstring, ok?

* extraction : dict -> passed to XY
@joleroi

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Apr 27, 2017

Contributor

Left a bunch of nitpicky inline comments.

Like in the good old days ... I think I will go back to directly pushing to master 😉

Contributor

joleroi commented Apr 27, 2017

Left a bunch of nitpicky inline comments.

Like in the good old days ... I think I will go back to directly pushing to master 😉

@joleroi

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Apr 27, 2017

Contributor

I adressed your comments, merging.

Contributor

joleroi commented Apr 27, 2017

I adressed your comments, merging.

@joleroi joleroi merged commit b8139fe into gammapy:master Apr 27, 2017

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@joleroi joleroi deleted the joleroi:spec_pipe2 branch Apr 27, 2017

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