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 model registries and Model.from_dict method #2338

Merged
merged 5 commits into from Sep 3, 2019

Conversation

@QRemy
Copy link
Contributor

commented Sep 3, 2019

add spatial_models dict to gammapy.image.models
add spectral_modelcs dict to gammapy.spectrum.models
add tag to model
add from_dict() to model
modify serialization.io to use model.from_dict()

add spatial_models dict to gammapy.image.models
add spectral_modelcsdict to gammapy.spectrum.models
add tag to model
add from_dict() to model
modify io to init model.from_dict()
Copy link
Member

left a comment

@QRemy - Please run make black

I think we want the tag at the class level, not in __init__.
But that's also the case for the parameters.
So we can just move them together later.

gammapy/spectrum/models.py Outdated Show resolved Hide resolved
@cdeil cdeil self-assigned this Sep 3, 2019
@cdeil cdeil added the cleanup label Sep 3, 2019
gammapy/spectrum/models.py Outdated Show resolved Hide resolved
QRemy added 2 commits Sep 3, 2019
@cdeil

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

Copy link
Member

left a comment

Thanks @QRemy, I've left a few small comments. Once those are addressed the PR is ready to merge from my side.

gammapy/image/models/__init__.py Outdated Show resolved Hide resolved
gammapy/image/models/core.py Outdated Show resolved Hide resolved
gammapy/spectrum/models.py Outdated Show resolved Hide resolved
@adonath adonath added this to the 0.14 milestone Sep 3, 2019
@QRemy

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2019

@QRemy - I see two test fails: https://gist.github.com/cdeil/f7058f47fb4417f73f4feb24f5a40616

I removed the previous .from_dict() from SpectralModel so the old syntax won't work now,
I updated the .from_dict() test and the example in analysis.py to fix this

QRemy added 2 commits Sep 3, 2019
@cdeil
cdeil approved these changes Sep 3, 2019
@cdeil cdeil changed the title add models registers and model.from_dict() Add model registries and Model.from_dict method Sep 3, 2019
@cdeil

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

@adonath - Merge?

@cdeil

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

I think all comments by @adonath and me are addressed, CI is green.
As discussed offline today, we'll iterate on this anyways in the next weeks.

Tomorrow I'll move this code to gammapy.modeling (see #2319), so merging this in now.

@cdeil cdeil merged commit da09aff into gammapy:master Sep 3, 2019
9 checks passed
9 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Scrutinizer 12 new issues, 6 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
gammapy.gammapy Build #20190903.12 succeeded
Details
gammapy.gammapy (DevDocs) DevDocs succeeded
Details
gammapy.gammapy (Lint) Lint succeeded
Details
gammapy.gammapy (Test Python36) Test Python36 succeeded
Details
gammapy.gammapy (Test Windows36) Test Windows36 succeeded
Details
gammapy.gammapy (Test Windows37) Test Windows37 succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.