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

Improve spatial models and add diffuse models #1386

Merged
merged 5 commits into from Apr 21, 2018

Conversation

Projects
None yet
2 participants
@joleroi
Contributor

joleroi commented Apr 20, 2018

  • Make all spatial model work with numpy arrays
  • Use gammapy.maps in TemplateModel
  • Accept path as input to read methods in gammapy.maps

@joleroi joleroi added the cleanup label Apr 20, 2018

@joleroi joleroi added this to the 0.8 milestone Apr 20, 2018

@joleroi joleroi self-assigned this Apr 20, 2018

@joleroi joleroi requested a review from cdeil Apr 20, 2018

@cdeil cdeil assigned cdeil and unassigned joleroi Apr 20, 2018

@cdeil cdeil changed the title from Update spatial models to Improve spatial models and add diffuse models Apr 21, 2018

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Apr 21, 2018

Member

@joleroi - Thanks!
I've continued a bit here. E.g. the point source model still had an if. And the return values from evaluate were sometimes in deg-2, sometimes sr-1, sometimes without unit. I've now changed to go to rad at the start, do the model evaluation without units, and return in sr-1. Merging now.

Several things here are preliminary and need more thought: should we take the quantity handling out of evaluate completely? How should diffuse models be normalised and should the diffuse model map unit be taken into account or not? In init we probably should take parameter objects and defer the quantity -> parameter conversion to the Parameter class, not do it in the spatial model inits.
We can sort these things out in future PRs.

Member

cdeil commented Apr 21, 2018

@joleroi - Thanks!
I've continued a bit here. E.g. the point source model still had an if. And the return values from evaluate were sometimes in deg-2, sometimes sr-1, sometimes without unit. I've now changed to go to rad at the start, do the model evaluation without units, and return in sr-1. Merging now.

Several things here are preliminary and need more thought: should we take the quantity handling out of evaluate completely? How should diffuse models be normalised and should the diffuse model map unit be taken into account or not? In init we probably should take parameter objects and defer the quantity -> parameter conversion to the Parameter class, not do it in the spatial model inits.
We can sort these things out in future PRs.

@cdeil cdeil merged commit 29f8ccc into gammapy:master Apr 21, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@@ -42,12 +41,6 @@ def __str__(self):
ss += '\n\t'.join(covar.pformat())
return ss
@staticmethod
@abc.abstractmethod

This comment has been minimized.

@joleroi

joleroi Apr 23, 2018

Contributor

Why did you remove this? Isn't the point of having an abstract base class that you define an API that the daughter classes have to follow?

@joleroi

joleroi Apr 23, 2018

Contributor

Why did you remove this? Isn't the point of having an abstract base class that you define an API that the daughter classes have to follow?

This comment has been minimized.

@cdeil

cdeil Apr 23, 2018

Member

PyCharm was complaining that the ABC had a different call signature than the derived classes. I think any static analysis tool wood. The way we've structured evaluate the parameters will always be different, so I removed this for now.

Overall I think it's a draw: putting it in the ABC makes Python complain if a derived class forgets to implement it. But then static analysis tools reporting argument mismatches is also bad, because once we leave such problems in our code base, it becomes much harder to use static analysis to point out the real issues.

@cdeil

cdeil Apr 23, 2018

Member

PyCharm was complaining that the ABC had a different call signature than the derived classes. I think any static analysis tool wood. The way we've structured evaluate the parameters will always be different, so I removed this for now.

Overall I think it's a draw: putting it in the ABC makes Python complain if a derived class forgets to implement it. But then static analysis tools reporting argument mismatches is also bad, because once we leave such problems in our code base, it becomes much harder to use static analysis to point out the real issues.

This comment has been minimized.

@joleroi

joleroi Apr 23, 2018

Contributor

I see, thanks for the explanation 👍

@joleroi

joleroi Apr 23, 2018

Contributor

I see, thanks for the explanation 👍

lat = 0 * u.deg
actual = template(lon, lat)[0]
desired = 0.00017506744188722223 / u.deg ** 2
# desired = 1.1553735159851262 / u.deg ** 2

This comment has been minimized.

@joleroi

joleroi Apr 23, 2018

Contributor

So do you know why we cannot reproduce the old test value?

@joleroi

joleroi Apr 23, 2018

Contributor

So do you know why we cannot reproduce the old test value?

This comment has been minimized.

@cdeil

cdeil Apr 23, 2018

Member

I didn't have a look at the old values. I don't think it's worth, becase a) there were onlly 1-2 users of this (Axel + ?) and the behaviour wasn't well specified anyways. E.g. I think the old version was applying normalisation on init, and integrating over pixels, which are two things I think this class shouldn't do. Note that I left some TODO in the class of things I wanted to discuss with you concerning diffuse models. This is just a rough first draft. Do you have time today or later this week?

@cdeil

cdeil Apr 23, 2018

Member

I didn't have a look at the old values. I don't think it's worth, becase a) there were onlly 1-2 users of this (Axel + ?) and the behaviour wasn't well specified anyways. E.g. I think the old version was applying normalisation on init, and integrating over pixels, which are two things I think this class shouldn't do. Note that I left some TODO in the class of things I wanted to discuss with you concerning diffuse models. This is just a rough first draft. Do you have time today or later this week?

@joleroi joleroi deleted the joleroi:update_models branch Apr 23, 2018

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Apr 23, 2018

Member

I forgot to update the 3D example scripts. Done in master in:
a05ffd9

Member

cdeil commented Apr 23, 2018

I forgot to update the 3D example scripts. Done in master in:
a05ffd9

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