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 map sum_over_axes #1362

Merged
merged 1 commit into from Apr 10, 2018
Merged

Fix map sum_over_axes #1362

merged 1 commit into from Apr 10, 2018

Conversation

@cdeil
Copy link
Member

@cdeil cdeil commented Apr 7, 2018

Yesterday a change to numpy.squeeze landed that caused this error in Gammapy master:
https://travis-ci.org/gammapy/gammapy/jobs/363544680#L3839

The problem is that we're passing a list of int to numpy.squeeze and it seems they now require tuple of int instead. That seems weird, I've left a comment here:
numpy/numpy#10826 (comment)

However, probable we should just change to tuple of int. The docs of numpy.squeeze as well as numpy.apply_over_axes and numpy.sum that we call with list of int currently for axis or axes all say "tuple of int" should be passed. So I've made that change in this PR. Let's see if that works with Numpy dev and all other versions we text in CI.

@cdeil cdeil added the bug label Apr 7, 2018
@cdeil cdeil added this to the 0.8 milestone Apr 7, 2018
@cdeil cdeil self-assigned this Apr 7, 2018
@cdeil
Copy link
Member Author

@cdeil cdeil commented Apr 7, 2018

There's probably a bug in our code, see comment from numpy/numpy#10826 (comment)

Your code was already broken - the list you were passing was being completely ignored. To keep the behavior you had before, you can call it with no arguments as squeeze()

I'll have a look tomorrow and see what's going on ...

@woodmd
Copy link
Member

@woodmd woodmd commented Apr 8, 2018

While you're at it you might drop the call to squeeze in hpxnd entirely and just use np.sum with the axis argument (i.e. as in wcsnd). I think I was using apply_over_axes/squeeze before I realized that np.sum had an axis argument.

@cdeil cdeil force-pushed the cdeil:squeeze branch from e3bde99 to b86124c Apr 10, 2018
@cdeil
Copy link
Member Author

@cdeil cdeil commented Apr 10, 2018

@woodmd - Yes! I amended the commit to also do the fix for hpxnd.

@cdeil
Copy link
Member Author

@cdeil cdeil commented Apr 10, 2018

We're in a rush to improve things before the CTA meeting next week. So I'm merging this now without adding an extra regression test. This was triggered by a fail, so we have some for of regression test on this already.

@cdeil cdeil merged commit e5acaf5 into gammapy:master Apr 10, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@cdeil cdeil changed the title Change np.squeeze calls to pass a tuple of int Fix map sum_over_axes Apr 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants