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

Fix sorting of node values in MapAxis #2126

Merged
merged 2 commits into from May 2, 2019

Conversation

2 participants
@AtreyeeS
Copy link
Member

commented Apr 26, 2019

This PR addresses #2061 . The MapAxis are now enforced to be sorted during input.
The question of how to deal with labelled axis such as IRF classes or MC ids is left open for the time being.

@AtreyeeS AtreyeeS requested a review from adonath Apr 26, 2019

@AtreyeeS AtreyeeS self-assigned this Apr 26, 2019

@AtreyeeS AtreyeeS added the cleanup label Apr 26, 2019

@AtreyeeS AtreyeeS added this to To do in Map analysis via automation Apr 26, 2019

@AtreyeeS AtreyeeS added this to the 0.12 milestone Apr 26, 2019

Map analysis automation moved this from To do to In progress May 2, 2019

@adonath
Copy link
Member

left a comment

Thanks @AtreyeeS, I have one minor comment, otherwise it looks good to me...

@@ -495,6 +497,8 @@ def from_nodes(cls, nodes, **kwargs):
raise ValueError("Nodes array must have at least one element.")
if len(nodes) != len(np.unique(nodes)):
raise ValueError("MapAxis: node values must be unique")
if ~(np.all(nodes == np.sort(nodes)) or np.all(nodes[::-1] == np.sort(nodes))):

This comment has been minimized.

Copy link
@adonath

adonath May 2, 2019

Member

Maybe move this statement to MapAxis.__init__() to avoid the duplication in from_edges() and .from_nodes()?

This comment has been minimized.

Copy link
@AtreyeeS

AtreyeeS May 2, 2019

Author Member

Thanks @adonath ! Done

@adonath

adonath approved these changes May 2, 2019

@adonath adonath merged commit 8e8a277 into gammapy:master May 2, 2019

9 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Scrutinizer Analysis: 1 updated code elements – Tests: passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
gammapy.gammapy Build #20190502.3 succeeded
Details
gammapy.gammapy (DevDocs) DevDocs succeeded
Details
gammapy.gammapy (Lint) Lint succeeded
Details
gammapy.gammapy (Test Python35) Test Python35 succeeded
Details
gammapy.gammapy (Test Windows35) Test Windows35 succeeded
Details
gammapy.gammapy (Test Windows37) Test Windows37 succeeded
Details

Map analysis automation moved this from In progress to Done May 2, 2019

@adonath

This comment has been minimized.

Copy link
Member

commented May 2, 2019

Thanks @AtreyeeS!

@AtreyeeS AtreyeeS referenced this pull request May 2, 2019

Closed

Should MapAxis be sorted? #2061

@adonath adonath changed the title Enforce sorting of MapAxis Fix sorting of node values in MapAxis May 27, 2019

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