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

Rewrite GaussianBand2D model #1107

Merged
merged 6 commits into from Aug 25, 2017
Merged

Conversation

@adonath
Copy link
Member

@adonath adonath commented Aug 23, 2017

This PR rewrites the GaussianBand2D model and adds it as property to the SourceCatalogHGPS class.

It also adds a high level test, that recomputes the flux values from the large scale emission model in the RSpec region of some HGPS sources. The value agrees within 1% with the one listed in the catalog.

@cdeil
cdeil approved these changes Aug 23, 2017
Copy link
Member

@cdeil cdeil left a comment

Left some minor inline comments (mostly a matter of taste).

Thanks!

y = self._splines[parname](glon.degree)
return y * self._units[parname]

def peak_brightness(self, glon):

This comment has been minimized.

@cdeil

cdeil Aug 23, 2017
Member

Matter of taste, but I would suggest instead of these 8? one-line methods, make _interpolate_parameter public and list available parameters in the class or this docstring?

This comment has been minimized.

@adonath

adonath Aug 24, 2017
Author Member

It's mostly a question of the API...in this case I found it difficult, because this class is very much in between other model and data class we have. I decided to push this class more into the direction of a data class (I actually thought about renaming it to LargeScaleComponentHGPS...) and have a convenience layer that we also implemented for the FluxPoints, EventList or SourceCatalog class: the actual data is stored in a .table attribute and then we provide easy access to the most important quantities including unit handling etc.

This comment has been minimized.

@cdeil

cdeil Aug 24, 2017
Member

My suggestion was just because the 8 one-line methods don't add extra functionality and I slightly prefer the smaller API. But it's not really a big gain because then you have to list the available parameters in a single docstring. You choose and merge as you like.

assert isinstance(model, Gaussian1D)
assert_allclose(model.parameters, [0, -1, 0.4])
glon = Angle(-30, unit='deg')
assert_quantity_allclose(self.model.peak_brightness(glon), 0 * u.Unit('cm-2 s-1 sr-1'))

This comment has been minimized.

@cdeil

cdeil Aug 23, 2017
Member

Pick some position for assert where value is != 0? Would that test a bit more or doesn't it matter?

# regions with ones listed in the catalog, agreement is <1%
ls_model = self.cat.large_scale_component

for name in ['HESS J1837-069', 'HESS J1809-193', 'HESS J1841-055']:

This comment has been minimized.

@cdeil

cdeil Aug 23, 2017
Member

Use pytest.parametrize instead of for loop to make the test function a bit shorter and less indented?
(and if one fails, the error message will be better)

@cdeil
Copy link
Member

@cdeil cdeil commented Aug 25, 2017

@adonath - Merge now?

I think you did fix the fails in master from the regions changes here:

TypeError: contains() got an unexpected keyword argument 'wcs'

I'd like to get rid of those in master (and also this PR is ready, no)?

@cdeil cdeil added this to the 0.7 milestone Aug 25, 2017
@adonath adonath merged commit 3214784 into gammapy:master Aug 25, 2017
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@adonath adonath deleted the adonath:rewrite_gaussian_band_model branch Nov 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants