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

Add morphology models as Astropy models #122

Merged
merged 4 commits into from Jul 15, 2014

Conversation

Projects
None yet
2 participants
@adonath
Member

adonath commented Jun 5, 2014

Implementation of morphology models as astropy models and testing.

y position center of the shell
R_in : float
Inner radius of the shell
R_out : float

This comment has been minimized.

@cdeil

cdeil Jun 5, 2014

Member

It's better to use width as parameter instead of outer radius, because then the constraint r_out > r_in (many fitters can't handle that) becomes simpler: r_in > 0 and width > 0 ... and the correlation between the fit parameters is smaller.

See here for an example: https://github.com/gammalib/gammalib/blob/devel/src/model/GModelSpatialRadialShell.cpp

Parameters
----------
amplitude : float
Peak value of the shell function

This comment has been minimized.

@cdeil

cdeil Jun 5, 2014

Member

I think this is not really the peak intensity value?
Maybe change description to be correct (maybe by being vague or more explicit)?

See Also
--------
Sphere2D, Delta2D, Gaussian2D

This comment has been minimized.

@cdeil

cdeil Jun 5, 2014

Member

Does this generate links in the HTML docs?
At least Gaussian2D is in astropy, right?

# Because np.select evaluates on the whole rr array
# we have to catch the invalid value warnings
np.seterr(invalid='ignore')

This comment has been minimized.

@cdeil

cdeil Jun 5, 2014

Member

Don't you have to set this back?
Is it possible to use this as in a with statement to automatically take care of setting it back?

y_0 : float
y position center of the sphere
R_0 : float
Inner radius of the sphere

This comment has been minimized.

@cdeil

cdeil Jun 5, 2014

Member

Inner radius -> Radius.

Parameters
----------
amplitude : float
Peak value of the sphere function

This comment has been minimized.

@cdeil

cdeil Jun 5, 2014

Member

Again, it's not the peak value, is it?

return amplitude * values
class Sphere2D(Fittable2DModel):

This comment has been minimized.

@cdeil

cdeil Jun 5, 2014

Member

The Sphere2D is the same as the Shell2D with R_in = 0 fixed, no?
If you want it as a convenience, maybe at least mention that in the docstring?

Parameters
----------
amplitude : float
Peak value of the sphere function

This comment has been minimized.

@cdeil

cdeil Jun 5, 2014

Member

Copy & paste error ... you mention "sphere" several times in this Delta2D docstring.

}
class TestMorphologyModels(object):

This comment has been minimized.

@cdeil

cdeil Jun 5, 2014

Member

This should be changed in Astropy core to be re-usable.
If you want to merge this now, please add a TODO: line saying that it should be removed here when available in Astropy ... or put it in gammapy/extern if it's a copy of code from Astropy.

@cdeil

This comment has been minimized.

Member

cdeil commented Jun 5, 2014

Well, this should probably all be in astropy.modeling, but let's merge it in gammapy to have it available now.

I'm not sure this is a good way to implement the Delta2D model ... I once looked at how Sherpa does it, but don't remember any more now.

Somehow GammaLib manages to fit point source locations with sub-pixel precision and the flux correctly using this model which can be used with any PSF:
https://github.com/gammalib/gammalib/blob/devel/src/model/GModelSpatialPointSource.cpp#L260
But I never looked at how it's implemented.

@cdeil

This comment has been minimized.

Member

cdeil commented Jun 5, 2014

The travis-ci build is broken in gammapy master at the moment: #123
Let's wait until tomorrow to merge this.

@adonath

This comment has been minimized.

Member

adonath commented Jun 10, 2014

There is still an issue with the external link to astropy.modeling.functional_models.Gaussian2D in the docs, that I can't resolve. @cdeil Any ideas?

@cdeil

This comment has been minimized.

Member

cdeil commented Jun 10, 2014

This was changed in the Astropy sphinx setup ... you should now always put links to the user-facing location (not the actual file / module location) ... in this case astropy.modeling.models.Gaussian2D.

@cdeil cdeil added feature labels Jun 10, 2014

@cdeil cdeil added this to the 0.1 milestone Jun 10, 2014

@cdeil

This comment has been minimized.

Member

cdeil commented Jul 4, 2014

@adonath Can you please finish this pull request (at the moment there's e.g. a tmp/test.py file attached)?
I think it makes sense to get this into gammapy now and maybe try to get it into Astropy before the 1.0 release.

@adonath

This comment has been minimized.

Member

adonath commented Jul 14, 2014

I've updated this PR, but it seems like the Travis builds are broken (not failing, but broken...). @cdeil could you take a look at this?

@cdeil

This comment has been minimized.

Member

cdeil commented Jul 14, 2014

Yes, our travis-ci build is broken and will be until we update to use astropy-helpers (see #126), which I plan to do when Astropy 0.4 is released later this week.

I'll try this on my Macbook now.

np.seterr(invalid='ignore')
values = np.select([rr <= rr_0, rr > rr_0], [np.sqrt(rr_0 - rr), 0])
np.seterr(invalid='warn')

This comment has been minimized.

@cdeil

cdeil Jul 14, 2014

Member

This will change the global setting of the user to warn.
I think you can use np.errstate instead?

with np.errstate(invalid='ignore'):
    values = np.select([rr <= rr_0, rr > rr_0], [np.sqrt(rr_0 - rr), 0])

Ideally the code should be re-written to handle the different cases differently using indexing, but I'm not sure how to do it with good performance and to avoid evaluating np.sqrt for the cases of negative numbers.

See Also
--------
Shell2D, Sphere2D, astropy.modeling.functional_models.Gaussian2D

This comment has been minimized.

@cdeil

cdeil Jul 14, 2014

Member

Sphinx links now always should point to where the user is supposed to import from, not the file location.
In this case

astropy.modeling.Gaussian2D

The Sphinx build currently shows a few warnings that should also be fixed:

/private/tmp/gammapy/docs/api/gammapy.morphology.Delta2D.rst:7: WARNING: py:class reference target not found: astropy.modeling.core.Fittable2DModel
/private/tmp/gammapy/docs/api/gammapy.morphology.Shell2D.rst:7: WARNING: py:class reference target not found: astropy.modeling.core.Fittable2DModel
/private/tmp/gammapy/docs/api/gammapy.morphology.Sphere2D.rst:7: WARNING: py:class reference target not found: astropy.modeling.core.Fittable2DModel
@cdeil

This comment has been minimized.

Member

cdeil commented Jul 14, 2014

Looks like currently the eval method of Sphere2D and Shell2D is not tested:

$ python setup.py test -V -t  gammapy/morphology/tests/test_shapes.py --coverage
$ open htmlcov/index.html 

Can you please add a test?

@adonath

This comment has been minimized.

Member

adonath commented Jul 15, 2014

I've adressed your comments and added a test for the eval method. Is it now ready to merge?

@cdeil

This comment has been minimized.

Member

cdeil commented Jul 15, 2014

Thanks!

cdeil added a commit that referenced this pull request Jul 15, 2014

Merge pull request #122 from adonath/shell2d_model_#115
Implementation of morphology models as astropy models and testing

@cdeil cdeil merged commit 8933045 into gammapy:master Jul 15, 2014

1 check failed

continuous-integration/travis-ci The Travis CI build could not complete due to an error
Details

@adonath adonath referenced this pull request Jul 16, 2014

Closed

Implement Shell2D model #115

@cdeil cdeil changed the title from Implementation of morphology models as astropy models and testing to Add morphology models as Astropy models Apr 8, 2015

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