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

Add Fermi-LAT 3FHL spatial models #1013

Merged
merged 12 commits into from May 4, 2017
Merged

Conversation

@adonath
Copy link
Member

@adonath adonath commented May 3, 2017

This PR adds the .spatial_model attribute to SourceCatalogObject3FHL. The corresponding extended sources template archive is added in gammapy/gammapy-extra#78.

@adonath adonath added the feature label May 3, 2017
@adonath adonath added this to the 0.7 milestone May 3, 2017
@adonath adonath self-assigned this May 3, 2017
@adonath adonath changed the title Add Fermi_LAt 3FHL spatial models Add Fermi-LAT 3FHL spatial models May 3, 2017
@adonath adonath merged commit 3a38a1e into gammapy:master May 4, 2017
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@adonath
Copy link
Member Author

@adonath adonath commented May 4, 2017

Travis-CI test fails are unrelated

@bsipocz
Copy link
Member

@bsipocz bsipocz commented May 5, 2017

@adonath - Actually this one messed up the docs builds on travis, I suspect it's something in Template2D that causes the timeout and hangs sphinx.

@adonath adonath mentioned this pull request May 7, 2017
Copy link
Member

@cdeil cdeil left a comment

I realise I'm late here (have been sick since Wednesday), but I still did a review. @adonath - Can you please do a follow-up PR taking the comments into account? (If no, I'll make a reminder issue for myself and do it next week.)

pars['x_0'] = glon.value
pars['y_0'] = glat.value
return Delta2D(**pars)
else:

This comment has been minimized.

@cdeil

cdeil May 7, 2017
Member

Maybe remove the else and dedent the rest?

raise ValueError('Not a valid spatial model{}'.format(morph_type))

@property
def pointlike(self):

This comment has been minimized.

@cdeil

cdeil May 7, 2017
Member

Uncle Bob and the other clean code guys suggest to put is or has for boolean attributes, i.e. in this case pointlike -> is_pointlike. I think it makes sense and we should change here (and in other cases where needed in Gammapy.

pars['y_mean'] = glat.value
pars['x_stddev'] = de['Model_SemiMajor'].to('deg').value
pars['y_stddev'] = de['Model_SemiMajor'].to('deg').value
pars['amplitude'] = amplitude * 1 / (2 * np.pi * pars['x_stddev'] ** 2)

This comment has been minimized.

@cdeil

cdeil May 7, 2017
Member

Such amplitude to integral conversions belong on the model classes, to avoid duplicating the formulae in our codebase, no? Maybe as a static helper function to compute before __init__ or as an option in init or a method to call after init?

In this case just taking x_stddev and not considering y_stddev is a bug for asymm cases, no?

See Also
--------
Shell2D, Sphere2D, `~astropy.modeling.models.Gaussian2D`

This comment has been minimized.

@cdeil

cdeil May 7, 2017
Member

Remove empty line.

template = SkyImage.read(filename)
return cls(template)

def evaluate(self, x, y, amplitude):

This comment has been minimized.

@cdeil

cdeil May 7, 2017
Member

Add docstring and say what x / y is here. Pix or world coords? Units?

return cls(template)

def evaluate(self, x, y, amplitude):
coord = SkyCoord(x, y, frame='galactic', unit='deg')

This comment has been minimized.

@cdeil

cdeil May 7, 2017
Member

Hard-coding to galactic here is bad, no?
Avoid creating a SkyCoord object possible?

return amplitude * values

@property
def bounding_box(self):

This comment has been minimized.

@cdeil

cdeil May 7, 2017
Member

Move bounding_box to the SkyImage class?
Should be useful in other contents as well, no?

This is what we have:
http://docs.gammapy.org/en/latest/api/gammapy.image.SkyImage.html#gammapy.image.SkyImage.footprint
I thought this just computes the corners and that's what the docstring says.
Here I see that also e.g. center is computed.
@adonath - Did you add this?
I think this is bad, there can just be separate properties like center, or whatever is needed, and footprint should just return what the name says: the footprint (not center, width, height).

center = footprint['center']
x_0 = center.data.lon.wrap_at('180d').deg
y_0 = center.data.lat.deg
return ((y_0 - height / 2, y_0 + height / 2),

This comment has been minimized.

@cdeil

cdeil May 7, 2017
Member

Tuples are bad, it's so easy to get the order wrong. And there's no docstring here explaining what this computes at all and what is returned.

'/Templates/HESSJ1841-055.fits')
template = Template2D.read(filename)
assert_allclose(template(26.7, 0), 1.1553735159851262)

This comment has been minimized.

@cdeil

cdeil May 7, 2017
Member

Please add an assert that exercises bounding_box and establishes the current result (to make it easier to refactor or change the code that computes it in the future).

@adonath adonath deleted the adonath:fermi_lat_spatial_models branch Nov 20, 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

3 participants