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 new SpatialModel base class #1376

Merged
merged 2 commits into from Apr 13, 2018

Conversation

Projects
None yet
4 participants
@joleroi
Contributor

joleroi commented Apr 13, 2018

Introduce SpatialModels based on Parameter and ParameterList to be used in a cube fit. For now the model names begin with Sky in order to avoid naming conflicts with older implementations.

@joleroi joleroi added the feature label Apr 13, 2018

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

@joleroi joleroi self-assigned this Apr 13, 2018

@joleroi joleroi requested review from cdeil and adonath Apr 13, 2018

"""Evaluate the model (static function)."""
exp_ = (lon - lon_mean) ** 2 + (lat - lat_mean) ** 2
val = amplitude * np.exp((- 1 / (2 * sigma ** 2)) * exp_)
return val

This comment has been minimized.

@registerrier

registerrier Apr 13, 2018

Contributor

Wouldn't it better to use erf to limit sampling effects when sigma is close to bin size?

This comment has been minimized.

@joleroi

joleroi Apr 13, 2018

Contributor

I don't know, you tell me 😁 I don't understand how sampling effects play a role here, this model is meant to be evaluated at a certain position.

This comment has been minimized.

@cdeil

cdeil Apr 13, 2018

Member

@registerrier - This is on the sphere, so there is no analytical formula for integrating over pixels (erf is only for cartesian pixel grids).

Of course we'll have problems fitting small sources, but for now we do the simple thing to get a first 3D and then we re-discuss how / where to add special cases.

@adonath

I also discussed offline with @cdeil. None of my comments are important for now, but should be kept in mind for later (especially how to handle coordinates and frames...)

Parameters
----------
lon_mean : `~astropy.coordiantes.Longitude`

This comment has been minimized.

@adonath

adonath Apr 13, 2018

Member

I think the lon_mean and lat_mean parameter names are very specific to a Gaussian. I'd prefer a more generic name, that we can use for all spatial model, such as lon, lat or even x_0 or y_0 if we do not want to stick with Galactic coordinates.

This comment has been minimized.

@cdeil

cdeil Apr 13, 2018

Member

I'll change to lon and lat before merging.

This comment has been minimized.

@joleroi

joleroi Apr 16, 2018

Contributor

The evaluation coordinates are already called lon and lat, this might lead to confusion. I'll change to x_0 and y_0 in a future PR

This comment has been minimized.

@cdeil

cdeil Apr 16, 2018

Member

I think it's the other way: using x / y for lon / lat will lead to confusion.
I would prefer to keep lon and lat in the name, and only use x / y for pixel coordinates.
Whether you append "mean" or "center" or "0" or nothing is just a matter of taste.
I would just put "lon" and "lat", but @joleroi - for now you're in charge to just put names you like, and then when the dust has settled, next week we'll discuss names and implementation, hopefully with @registerrier and @adonath if they are available.

return copy.deepcopy(self)
class SkyGaussian2D(SkySpatialModel):

This comment has been minimized.

@adonath

adonath Apr 13, 2018

Member

Maybe rename to SkySymmetricGaussian to reflect the symmetry of the model in the name?

This comment has been minimized.

@cdeil

cdeil Apr 13, 2018

Member

Not sure if we want to add a separate class later for elongated or add it to this one.
So I'll keep name as-is. This is just to have a working 3D fit ASAP, this is not supposed to be the final / stable API.

])
@staticmethod
def evaluate(lon, lat, lon_mean, lat_mean, sigma):

This comment has been minimized.

@adonath

adonath Apr 13, 2018

Member

I'm not really sure how we should handle the sky coordinates for model evaluation. I do not like the fact that we are bound to Galactic coordinates here, but it is probably the easiest solution. Alternatively one could define the coordinate system during initialization of the model by introducing a frame attribute (similar to SkyCoord) and store it as class attribute. One could then think about accepting a SkyCoord object in the __call__() method:

model = SkyGaussian2D(x=0*u.deg, y=0*u.deg, frame='galactic')
image = SkyImage()
values = model(image.coordinates())

Alternatively keep as is and later introduce a convenience property SpatialModel.position, which returns a SkyCoord object.

This comment has been minimized.

@cdeil

cdeil Apr 13, 2018

Member

Good point. But let's postpone frame-handling to a future PR.
For now, it'll only be possible to fit (lon, lat) in the frame of the map you're fitting, which you can make in RADEC or GALACTIC or any other system you like.
It's not clear to me where the frame transformations should occur yet, something to be discussed later.

This comment has been minimized.

@cdeil

cdeil Apr 13, 2018

Member

The basic idea is to only use Numpy or maybe Quantity like here in model evaluation, and to use SkyCoord, Time, ... not in the model evaluate / likelihood fit. We could add convenience properties though, e.g. a skycoord to get and set center of the spatial models.
But then again, the model would probably also have to do frame handling, ... let's continue that discussion after we have a first working 3D fit and a few results.
(Gammapy CTA presentation next week Thursday morning)

@cdeil

cdeil approved these changes Apr 13, 2018

@cdeil cdeil merged commit 3b4067d into gammapy:master Apr 13, 2018

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

This comment has been minimized.

Member

cdeil commented Apr 13, 2018

@joleroi - Thanks!

@joleroi joleroi deleted the joleroi:spatialmodel branch Apr 16, 2018

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