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

Update catalog to new model classes #1387

Merged
merged 3 commits into from Apr 26, 2018

Conversation

Projects
None yet
3 participants
@cdeil
Copy link
Member

cdeil commented Apr 23, 2018

This PR updates the catalog code to use the new spatial model classes.

This is work in progress, it's currently failing.

I noticed that two things are missing before this can be finalised:

  1. an elongated Gaussian model (should be added as a separate class for efficiency reasons)
  2. the + operator for the spatial models, or a special class representing sum of models

The following changes should be done here also:

  1. check if the 3FGL and 3FHL spatial model methods are identical and avoid code duplication
  2. add tests for new sky_model methods
  3. add asserts in spatial model tests for gamma-cat and Fermi-LAT (no good tests in place so far)

cc @joleroi

@cdeil cdeil added the cleanup label Apr 23, 2018

@cdeil cdeil added this to the 0.8 milestone Apr 23, 2018

@cdeil cdeil self-assigned this Apr 23, 2018

@joleroi

This comment has been minimized.

Copy link
Contributor

joleroi commented Apr 23, 2018

the + operator for the spatial models, or a special class representing sum of models

FYI: For the spectral model I have chosen the second option. If you have no strong point against it, I would suggest to do the same here for consistency

@cdeil

This comment has been minimized.

Copy link
Member Author

cdeil commented Apr 23, 2018

@joleroi - Yes, I would suggest to implement a plus operator for now for spatial and sky models also, like you did for spectral models. Is code re-use advisable e.g. via a mixin class or would you copy & paste?

This is not super efficient, e.g. if there was a special SumModel([list of models]) e.g. the model evaluations could be parallelised and optimised better, because the model evaluator has the info that the components are independent. It's also slightly easier to implement and + is by far the most important use case. But probably it's best to do the arithmetic model now, and only consider adding that special case later.

@joleroi

This comment has been minimized.

Copy link
Contributor

joleroi commented Apr 23, 2018

Is code re-use advisable e.g. via a mixin class

No, at the moment its hard coded to SpectralModel I can try to make it reusable, depending on when you need it

@cdeil

This comment has been minimized.

Copy link
Member Author

cdeil commented Apr 23, 2018

I would suggest to just do minimal copy & paste for now to make + work this week for SpatialModel and SkyModel, and open a reminder issue for v0.9 to make this nice for later.

@cdeil

This comment has been minimized.

Copy link
Member Author

cdeil commented Apr 26, 2018

I've finished the change of gammapy.catalog to use the new model classes here. This is ready to merge if CI passes. There's plenty of remaining open TODO that I left as inline comments, but this is as far as I want to go in this PR. We can then continue with this in new PRs next week.

Some comments:

@joleroi - added a little class SkySumModel to hold the multi-Gauss HGPS sources. I just put it in hess.py for now, and it's not properly implemented, I don't think evaluate would work, it's just to be able to finish this PR. We can then discuss tomorrow how to do this properly and finish this up next week.

@adonath - I xfailed the image catalog estimator tests for now. The tests should be rewritten to use SkyMapEvaluator, and then the old ImageEstimator can be removed.

The API to fill parameter errors is cumbersome, and in many cases only model values are filled, parameter errors and tests / asserts on those values still need to be added. To be done after #1398 .

@cdeil cdeil merged commit 436b154 into gammapy:master Apr 26, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@@ -107,6 +107,16 @@ def test_spectral_model(self, ref):
assert_quantity_allclose(dnde, ref['dnde'])
assert_quantity_allclose(dnde_err, ref['dnde_err'])

@pytest.mark.parametrize('ref', SOURCES_3FGL, ids=lambda _: _['name'])

This comment has been minimized.

@bsipocz

bsipocz Jul 9, 2018

Member

@cdeil - I have tons of issues with these tests when checking on the deprecation warnings.

Actually I don't see how this ever worked as SOURCES_3FGL above has no 'name' key just 'idx', but then if I change name to idx it all fall over. Are there names for those sources that can be used here? Why are these tests pass for you?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.