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 SkyModels read and write methods #2651

Merged
merged 2 commits into from Dec 2, 2019
Merged

Add SkyModels read and write methods #2651

merged 2 commits into from Dec 2, 2019

Conversation

@cdeil
Copy link
Member

cdeil commented Dec 2, 2019

In #2621, #2631 and #2642 we change AnalysisConfig and Datasets to have a read and write method, which calls from_yaml and to_yaml and does the I/O.

This two-step way to do I/O, and to have read and write as the methods that hit the disk is a good pattern in general, it's also done e.g. in Astropy with Table.read and Table.write.

Suggest to do the same in SkyModels, to rename the existing from_yaml and to_yaml there to read and write (and to keep the from_yaml and to_yaml and return YAML strings, as interface for users that want to work with YAML in-memory (e.g. to print in a notebook, or to send to a database or whatever they like).

Example how it should look:

@classmethod
def read(cls, path):
"""Reads from YAML file."""
config = read_yaml(path)
return AnalysisConfig(**config)

@QRemy @adonath - Thoughts?

@cdeil cdeil added this to the 0.15 milestone Dec 2, 2019
@cdeil cdeil added this to To do in gammapy.modeling via automation Dec 2, 2019
@cdeil

This comment has been minimized.

Copy link
Member Author

cdeil commented Dec 2, 2019

@adonath - See commits attached here. Thoughts?

I'll run make docs-all now locally to see if I missed any tutorial notebooks to update.

@adonath
adonath approved these changes Dec 2, 2019
Copy link
Member

adonath left a comment

Thanks @cdeil! Looks good to me...please merge whenever you think it's ready.

@cdeil cdeil merged commit 5da4201 into gammapy:master Dec 2, 2019
7 of 10 checks passed
7 of 10 checks passed
greeting
Details
Scrutinizer Running
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
gammapy.gammapy in progress
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
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
gammapy.modeling automation moved this from To do to Done Dec 2, 2019
@cdeil

This comment has been minimized.

Copy link
Member Author

cdeil commented Dec 2, 2019

@AtreyeeS @registerrier - You can now go ahead and put examples of SkyModels.read, SkyModels.write, SkyModels.from_yaml and SkyModels.to_yaml in the docs.

@registerrier - Specifically here I would suggest to show SkyModels.from_yaml:
https://docs.gammapy.org/dev/notebooks/analysis_1.html#Model-fitting

It's very typical that users have 2 or more model components. In that case they should use SkyModels I/O, not SkyModel I/O. For ctools, I'm not even sure there is an I/O on the individual model object. I guess there is, but it's never advertised in the docs, in Fermi ST and ctools there's always one standard way shown, to have a "source library" with one or more sources. I suggest we do the same, teach it that way, or users will be confused.

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