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 Datasets read and write methods #2631

Merged
merged 6 commits into from Nov 29, 2019
Merged

Conversation

QRemy
Copy link
Contributor

@QRemy QRemy commented Nov 29, 2019

Change names of datasets serialisation methods
datasets.to_yaml() -> datasets.write()
datasets.from_yaml() -> datasets.read()

@QRemy QRemy requested a review from adonath November 29, 2019 12:40
@QRemy QRemy added the cleanup label Nov 29, 2019
datasets.to_yaml() -> datasets.write()
datasets.from_yaml() -> datasets.read()
@cdeil
Copy link
Contributor

cdeil commented Nov 29, 2019

@QRemy - Please when opening a PR, always set a project, milestone and assignee. Assignee is the guy you want to review and merge your PR, in this case @adonath or me is fine.

If you don't have permission to set those things
let me know and I'll add you to the Gammapy maintainers team.

@QRemy QRemy added this to In progress in gammapy.modeling via automation Nov 29, 2019
@QRemy QRemy added this to the 0.15 milestone Nov 29, 2019
cdeil
cdeil previously approved these changes Nov 29, 2019
@adonath adonath assigned cdeil and unassigned adonath Nov 29, 2019
@cdeil
Copy link
Contributor

cdeil commented Nov 29, 2019

Probably for consistency, we should call make_path in read, just like you do in write in https://github.com/gammapy/gammapy/pull/2631/files#diff-8b37a3d39becd8c4acbc3951517b883fR182 and like we do in all our read methods.

What this will achieve is that variable names are expanded, i.e. we can use Datasets.read("$GAMMAPY_DATA/some/file.yaml") in tutorials and the env var will be expanded.
I'm not sure it's a good pattern, but we have it in basically all our read methods, so Datasets.read should work the same way.

@cdeil cdeil removed the request for review from adonath November 29, 2019 13:15
Copy link
Contributor

@cdeil cdeil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@QRemy - Thank you!

@cdeil cdeil changed the title Change datasets serialisation methods names Add Datasets read and write methods Nov 29, 2019
@cdeil
Copy link
Contributor

cdeil commented Nov 29, 2019

Changed title to "Add Datasets read and write methods" which I think will be more useful for users to see in the changelog.

I'm merging this now. Obviously we don't use this enough in the docs. @AtreyeeS - Please try to use it in the first and second analysis tutorial and your benchmark scripts, and report if you find any issues.

@cdeil cdeil merged commit b5ea3dd into gammapy:master Nov 29, 2019
gammapy.modeling automation moved this from In progress to Done Nov 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants