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 catalog getitem to work with numpy int index #906

Merged
merged 4 commits into from Feb 22, 2017

Conversation

Projects
None yet
2 participants
@mirca
Contributor

mirca commented Feb 22, 2017

Does this suffice to close #886?

If so, does this need a change log entry?

cc @cdeil

@cdeil cdeil self-assigned this Feb 22, 2017

@cdeil cdeil added the bug label Feb 22, 2017

@cdeil cdeil added this to the 0.6 milestone Feb 22, 2017

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Feb 22, 2017

Member

@mirca - Thanks!

I just now remembered that I saw _is_int from astropy.io.fits.util this used in regions:
https://github.com/astropy/astropy/blob/master/astropy/io/fits/util.py#L781

They define it slightly differently, and I'd prefer we do it the same way (just because I don't know if there's any difference / details we're not aware of).

My suggestion would be that here in Gammapy, we don't call it directly, because it's private in Astropy and might change. Instead, let's define our own utility function

def _is_int(val):
    return isinstance(val, all_integer_types)

in gammapy/utils/array.py in exactly the same way they did in astropy.io.fits.util.py.

Then import it from this catalog file and call it.

Usually I put changelog entries after I merge something in Gammapy, but if you put it yourself, that is fine as well of course.

Member

cdeil commented Feb 22, 2017

@mirca - Thanks!

I just now remembered that I saw _is_int from astropy.io.fits.util this used in regions:
https://github.com/astropy/astropy/blob/master/astropy/io/fits/util.py#L781

They define it slightly differently, and I'd prefer we do it the same way (just because I don't know if there's any difference / details we're not aware of).

My suggestion would be that here in Gammapy, we don't call it directly, because it's private in Astropy and might change. Instead, let's define our own utility function

def _is_int(val):
    return isinstance(val, all_integer_types)

in gammapy/utils/array.py in exactly the same way they did in astropy.io.fits.util.py.

Then import it from this catalog file and call it.

Usually I put changelog entries after I merge something in Gammapy, but if you put it yourself, that is fine as well of course.

@mirca

This comment has been minimized.

Show comment
Hide comment
@mirca

mirca Feb 22, 2017

Contributor

Hi @cdeil, thanks for clarifying!
Let me know if anything else is needed.

Contributor

mirca commented Feb 22, 2017

Hi @cdeil, thanks for clarifying!
Let me know if anything else is needed.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Feb 22, 2017

Member

Perfect.
Thanks!

Member

cdeil commented Feb 22, 2017

Perfect.
Thanks!

@cdeil cdeil merged commit 73bfe65 into gammapy:master Feb 22, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@cdeil cdeil changed the title from ENH: enable catalog getitem to work with numpy int index to Enable catalog getitem to work with numpy int index Feb 22, 2017

@mirca mirca deleted the mirca:fix886 branch Feb 22, 2017

@cdeil cdeil changed the title from Enable catalog getitem to work with numpy int index to Fix catalog getitem to work with numpy int index Feb 22, 2017

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