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 map write for custom axis name #1433

Merged
merged 1 commit into from Jun 20, 2018

Conversation

Projects
None yet
2 participants
@cdeil
Member

cdeil commented Jun 19, 2018

This PR fixes issue #1429 reported by @registerrier .

The key change is to use

s = 'AXIS%i' % i if name == '' else name
colnames = [s, s + '_MIN', s + '_MAX']

in make_axes_cols so that if the axis has a name, that name is used for the colnames.

The change in the test to add an axis name='spam' revealed the issue and now serves as a regression test:

axes1 = [MapAxis(np.logspace(0., 3., 3), interp='log', name='spam')]

@registerrier @woodmd - Please review.

@cdeil cdeil added the bug label Jun 19, 2018

@cdeil cdeil added this to the 0.8 milestone Jun 19, 2018

@cdeil cdeil requested review from registerrier and woodmd Jun 19, 2018

@registerrier

This comment has been minimized.

Show comment
Hide comment
@registerrier

registerrier Jun 19, 2018

Contributor

With this implementation, when the axis name is energy, the behavior is then to have column names: ENERGY E_MIN and E_MAX whatever the convention gadf or fgst-template.

This seems good to me as it is likely transparent for the user anyway. This might be what is actually stated in [the definition of the BANDS HDU] (http://gamma-astro-data-formats.readthedocs.io/en/latest/skymaps/index.html#bands-hdu).

Contributor

registerrier commented Jun 19, 2018

With this implementation, when the axis name is energy, the behavior is then to have column names: ENERGY E_MIN and E_MAX whatever the convention gadf or fgst-template.

This seems good to me as it is likely transparent for the user anyway. This might be what is actually stated in [the definition of the BANDS HDU] (http://gamma-astro-data-formats.readthedocs.io/en/latest/skymaps/index.html#bands-hdu).

@registerrier registerrier merged commit d1b9447 into gammapy:master Jun 20, 2018

1 of 2 checks passed

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