Skip to content
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 `SpectrumDatasetOnOff.stack()` #2360

Merged
merged 10 commits into from Sep 11, 2019

Conversation

@registerrier
Copy link
Contributor

commented Sep 10, 2019

This PR adds a the stack() method on the SpectrumDatasetOnOff. The tests from the SpectrumDatasetOnOffStacker are reused to test the new method.

Once this PR is merged. The SpectrumDatasetOnOffStacker will be removed and replaced by a classmethod on SpectrumDatasetOnOff: SpectrumDatasetOnOff.stack_from_list(dataset_list) that will create a a stacked dataset from a list.

@registerrier registerrier requested a review from adonath Sep 10, 2019
@registerrier registerrier added this to To do in Spectrum analysis (1D) via automation Sep 10, 2019
@registerrier registerrier added this to the 0.14 milestone Sep 10, 2019
Copy link
Member

left a comment

Thanks @registerrier! I've left a few minor comments, otherwise looks good to me.

I think at some point we should the get rid of the SspectrumDatasetOnOffStacker. I'm happy to prepare a PR, if you want to focus on other things...

the dataset to stack to the current one
"""

def is_valid(dataset):

This comment has been minimized.

Copy link
@adonath

adonath Sep 11, 2019

Member

Maybe move this helper function outside? Or implement a helper property SpectrumDatasetOnOff.is_stackable?

This comment has been minimized.

Copy link
@registerrier

registerrier Sep 11, 2019

Author Contributor

Done

average_alpha = total_alpha.sum() / total_off.sum()

acceptance = np.ones_like(self.counts_off.data, dtype=float)
idx = np.where(total_off == 0)[0]

This comment has been minimized.

Copy link
@adonath

adonath Sep 11, 2019

Member

I think the np.where()call is not needed, you can directly define the mask as is_zero = total_off == 0

This comment has been minimized.

Copy link
@registerrier

registerrier Sep 11, 2019

Author Contributor

Done

gammapy/spectrum/dataset.py Show resolved Hide resolved
gammapy/spectrum/dataset.py Show resolved Hide resolved
gammapy/spectrum/dataset.py Show resolved Hide resolved
@adonath

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

Thanks @registerrier, I have no further comments...

@registerrier registerrier merged commit 1d89384 into gammapy:master Sep 11, 2019
9 checks passed
9 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Scrutinizer Analysis: 9 updated code elements – Tests: passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
gammapy.gammapy Build #20190911.1 succeeded
Details
gammapy.gammapy (DevDocs) DevDocs succeeded
Details
gammapy.gammapy (Lint) Lint succeeded
Details
gammapy.gammapy (Test Python36) Test Python36 succeeded
Details
gammapy.gammapy (Test Windows36) Test Windows36 succeeded
Details
gammapy.gammapy (Test Windows37) Test Windows37 succeeded
Details
Spectrum analysis (1D) automation moved this from To do to Done Sep 11, 2019
@registerrier

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2019

Tests passed. Merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
2 participants
You can’t perform that action at this time.