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 DataStore subset method #502

Merged
merged 5 commits into from Apr 11, 2016

Conversation

Projects
None yet
2 participants
@joleroi
Contributor

joleroi commented Apr 5, 2016

This PR introduces the functionality necessary to create a new DataStore from an existing one using a subset of observations.

@joleroi joleroi self-assigned this Apr 5, 2016

@joleroi joleroi added the feature label Apr 5, 2016

@cdeil cdeil added this to the 0.4 milestone Apr 5, 2016

Show outdated Hide outdated gammapy/data/data_store.py Outdated
Show outdated Hide outdated gammapy/data/data_store.py Outdated
@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Apr 5, 2016

Member

You should filter the obs and HDU index table as well, and store those as FITS files in outdir, no?
So that the new data store is complete on it's own.

Member

cdeil commented Apr 5, 2016

You should filter the obs and HDU index table as well, and store those as FITS files in outdir, no?
So that the new data store is complete on it's own.

Show outdated Hide outdated gammapy/data/data_store.py Outdated
@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Apr 5, 2016

Member

(I'm offline now ... thank you and good night!)

Member

cdeil commented Apr 5, 2016

(I'm offline now ... thank you and good night!)

Show outdated Hide outdated gammapy/data/tests/test_data_store.py Outdated
Show outdated Hide outdated gammapy/data/data_store.py Outdated
@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Apr 5, 2016

Member

(I didn't see the "WIP" in the title. I'll shut up now. Sorry!)

Member

cdeil commented Apr 5, 2016

(I didn't see the "WIP" in the title. I'll shut up now. Sorry!)

@joleroi

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Apr 6, 2016

Contributor

Hi, sorry this was more complicated than I though.

  • Can you please have a look and check if the test actually tests for a valid datastore?
  • Can you also have a look at the implementation, since you've written all the DataStore stuff (especially the HDUindex table). There's probably room for improvement.
Contributor

joleroi commented Apr 6, 2016

Hi, sorry this was more complicated than I though.

  • Can you please have a look and check if the test actually tests for a valid datastore?
  • Can you also have a look at the implementation, since you've written all the DataStore stuff (especially the HDUindex table). There's probably room for improvement.

@joleroi joleroi changed the title from WIP: DataStore subset to DataStore subset Apr 6, 2016

@joleroi

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Apr 6, 2016

Contributor

I left some comments where I found broken stuff. Can you judge if we still need all of this?

Contributor

joleroi commented Apr 6, 2016

I left some comments where I found broken stuff. Can you judge if we still need all of this?

@joleroi joleroi merged commit 35d4795 into gammapy:master Apr 11, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@joleroi joleroi deleted the joleroi:subdatastore branch Apr 11, 2016

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Apr 11, 2016

Member

Thanks!
(I'll look over the other PRs now and re-start travis-ci where it's useful.)

Member

cdeil commented Apr 11, 2016

Thanks!
(I'll look over the other PRs now and re-start travis-ci where it's useful.)

@cdeil cdeil changed the title from DataStore subset to Add DataStore subset method Apr 20, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment