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 SpectrumObservationStacker #740

Merged
merged 2 commits into from Oct 17, 2016

Conversation

Projects
None yet
2 participants
@joleroi
Contributor

joleroi commented Oct 14, 2016

This PR

  • refactors the stack method on SpectrumObservation into a stand-alone class SpectrumObservationStacker
  • add some test to test_observation because some functions where not tested/buggy

This was needed since the function required counts vectors, aeff, and edisp to be present which is not always the case. The class can stack all objects independently and is thus reusable for flux points computation methods (where one counts have to be stacked)

@joleroi joleroi added the cleanup label Oct 14, 2016

@cdeil cdeil added this to the 0.5 milestone Oct 14, 2016

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Oct 14, 2016

Member

Do you think it should be part of the public Gammapy API?
I.e. should it be listed in __all__ and appear in the docs?
Or will only few people need to use it directly?

Is there something specific I should review?
I'm very short on time, and generally +1 to put such algorithms in separate classes if they consist of several steps.

Member

cdeil commented Oct 14, 2016

Do you think it should be part of the public Gammapy API?
I.e. should it be listed in __all__ and appear in the docs?
Or will only few people need to use it directly?

Is there something specific I should review?
I'm very short on time, and generally +1 to put such algorithms in separate classes if they consist of several steps.

@joleroi

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Oct 14, 2016

Contributor

Or will only few people need to use it directly?

I guess I could be hidden in the public API (and exposed via a stack function on SpectrumObservationList)

Is there something specific I should review?

I guess it's fine, I will remove the class from the public API and then merge if tests pass

Contributor

joleroi commented Oct 14, 2016

Or will only few people need to use it directly?

I guess I could be hidden in the public API (and exposed via a stack function on SpectrumObservationList)

Is there something specific I should review?

I guess it's fine, I will remove the class from the public API and then merge if tests pass

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Oct 14, 2016

Member

I'm 50:50 on whether such classes should be part of the public API.

For experts it's nice to be able to find and read via the docs how spectral analysis works.
The concern is that beginners will get lost with too many classes.

Maybe we should err on the side of having many things in the API docs, at least for now?
One advantage would be that then the docstrings are built by Sphinx and checked for syntax / formatting.

Looking at the current docs for this class:
https://github.com/joleroi/gammapy/blob/c5d121c2a866ee0c603393281ca2393519e84b77/gammapy/spectrum/observation.py#L434
I think it would be good to have a little more in the class-level docstring.
Ideally a usage example that shows how to execute an example and access the results.
(my main point is that one has to read the code at the moment to find out what results are available, having that mentioned in the docstring, or putting them as properties that are computed on access would make this better, no?

Member

cdeil commented Oct 14, 2016

I'm 50:50 on whether such classes should be part of the public API.

For experts it's nice to be able to find and read via the docs how spectral analysis works.
The concern is that beginners will get lost with too many classes.

Maybe we should err on the side of having many things in the API docs, at least for now?
One advantage would be that then the docstrings are built by Sphinx and checked for syntax / formatting.

Looking at the current docs for this class:
https://github.com/joleroi/gammapy/blob/c5d121c2a866ee0c603393281ca2393519e84b77/gammapy/spectrum/observation.py#L434
I think it would be good to have a little more in the class-level docstring.
Ideally a usage example that shows how to execute an example and access the results.
(my main point is that one has to read the code at the moment to find out what results are available, having that mentioned in the docstring, or putting them as properties that are computed on access would make this better, no?

@joleroi

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Oct 14, 2016

Contributor

Maybe we should err on the side of having many things in the API docs, at least for now?

I don't really care. But I'd tend to have rather too much than too little stuff in the API. For beginners we just need to make nice tutorial notebooks

I think it would be good to have a little more in the class-level docstring.

Done

Contributor

joleroi commented Oct 14, 2016

Maybe we should err on the side of having many things in the API docs, at least for now?

I don't really care. But I'd tend to have rather too much than too little stuff in the API. For beginners we just need to make nice tutorial notebooks

I think it would be good to have a little more in the class-level docstring.

Done

@joleroi joleroi merged commit 5a257ec into gammapy:master Oct 17, 2016

2 checks passed

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

@joleroi joleroi deleted the joleroi:refactor_stack branch Oct 17, 2016

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