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 TableModel and ConstantModel output dimension #1871

Conversation

@registerrier
Copy link
Contributor

@registerrier registerrier commented Oct 9, 2018

This PR comes to correct #1869.
The problem arose because SkyModel.evaluate uses numpy broadcasting. The spectrum has to have a dimension (n,1,1) to be properly broadcast when multiplied with the spatial model.
This was not the case with the TableModel.
A numpy.reshape is added in TableModel.evaluate to ensure dimension is conserved.

A test is also added on spectral models to check that the dimensions of input and output quantities are equal. This required to modify ConstantModel which did not follow the rule.

@registerrier registerrier requested a review from cdeil Oct 9, 2018
@registerrier registerrier self-assigned this Oct 9, 2018
@registerrier registerrier added the bug label Oct 9, 2018
@registerrier registerrier added this to In progress in gammapy.modeling via automation Oct 9, 2018
@registerrier registerrier added this to the 0.9 milestone Oct 9, 2018
@registerrier registerrier requested a review from AtreyeeS Oct 9, 2018
@registerrier
Copy link
Contributor Author

@registerrier registerrier commented Oct 9, 2018

@AtreyeeS does it help solve #1868 or is it unrelated?

@registerrier registerrier changed the title Correct scaled grid interpolation dimension Correct TableModel and ConstantModel output dimension Oct 9, 2018
@AtreyeeS
Copy link
Member

@AtreyeeS AtreyeeS commented Oct 9, 2018

@AtreyeeS does it help solve #1868 or is it unrelated?

No, this does not. That might be unrelated. I will take a look

@cdeil
cdeil approved these changes Oct 9, 2018
Copy link
Member

@cdeil cdeil left a comment

@registerrier - Thanks!

I don't know about broadcasting in the spectral model evaluates. From your description and a quick look at the code, this change seems good. Suggest you go ahead and merge this in, and if needed, we can re-adjust later.

@AtreyeeS AtreyeeS merged commit db9275d into gammapy:master Oct 9, 2018
3 of 4 checks passed
3 of 4 checks passed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Scrutinizer Analysis: 23 new issues – Tests: passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
gammapy.modeling automation moved this from In progress to Done Oct 9, 2018
Copy link
Member

@AtreyeeS AtreyeeS left a comment

Thanks @registerrier !

@cdeil cdeil changed the title Correct TableModel and ConstantModel output dimension Fix TableModel and ConstantModel output dimension Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.