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

Implement DataStoreMaker for IACT DL3 indexing #2154

Merged
merged 3 commits into from May 24, 2019

Conversation

Projects
3 participants
@cdeil
Copy link
Member

commented May 23, 2019

This PR adds a DataStoreMaker and a DataStore.from_events_files method.

The use case (see docstring and test) is to make it possible to analyse IACT DL3 data if you don't have HDU&OBS index files, or to create HDU&OBS index files with Gammapy.

There are different formats in use how EVENTS and IRFs are linked, we probably want to support a few.

This initial version is supposed to work for data from CTA 1DC or from ctobssim, for the cases where CALDB is used to link to the IRFs. This was a feature request by @debaice , and pair-coded, to be able to analyse ctobssim output with Gammapy.

This PR is a step to address #1597 and #1303 .

I also would like to address #1255 here to allow datastore.get_observations() and to by default use all observations, by adding this:

if obs_id is None:
    obs_id = self.obs_table['OBS_ID'].data

For this case now where one makes a DataStore on the fly already for the data one wants, this is very common and convenient.

@adonath @lmohrmann @debaice or anyone interested in IACT data access / indexing - Thoughts?

@cdeil cdeil added the feature label May 23, 2019

@cdeil cdeil added this to the 0.12 milestone May 23, 2019

@cdeil cdeil requested review from adonath and lmohrmann May 23, 2019

@cdeil cdeil self-assigned this May 23, 2019

@cdeil cdeil added this to To do in Data via automation May 23, 2019

@lmohrmann
Copy link
Contributor

left a comment

This all seems like a very good idea 👍

I also like the option to retrieve all observations in a data store.

# TODO: check consistency for all EVENTS files and handle inconsistent case
# Transform times to first ref time? Or raise error for now?
# Test by combining some HESS & CTA runs?
meta["MJDREFI"] = 51544

This comment has been minimized.

Copy link
@lmohrmann

lmohrmann May 23, 2019

Contributor

Hard-coding these values here seems a bit ugly. Make it possible for the user to provide these somehow? Or could they be extracted from the simulated files?

This comment has been minimized.

Copy link
@cdeil

cdeil May 23, 2019

Author Member

We should definitely extract those from the EVENTS header (and check for consistency in the GTI header). We could add an option to let users set the ref time if the event file is "bad" and that info is missing. But it might be better to not put that in, forcing / encouraging people to create good EVENTS files.

If REF time is different in different EVENTS (like HESS & CTA), then it gets difficult.

I'll improve this (tomorrow or Monday) and ping you again for review of this PR.

@adonath
Copy link
Member

left a comment

Thanks @cdeil and @debaice! This is vey useful functionality. I've left a few optional inline comments.

My general comment would be to maybe try to simplify a little bit the code structure. I found the class solution with yield statements and caching overly complex. I think creating hdu and obs tables does not have to be super efficient for now, because it's typically done once and then the index tables are written to disk. If you have a few >1000 event files and it takes a few minutes to run is just fine. I guess a simple flat hierarchy helper method would do as well...but as the code is already written maybe just leave as is.

Show resolved Hide resolved gammapy/data/data_store.py
Show resolved Hide resolved gammapy/data/data_store.py Outdated
Show resolved Hide resolved gammapy/data/data_store.py Outdated
Show resolved Hide resolved gammapy/data/data_store.py

@cdeil cdeil force-pushed the cdeil:data-index branch from e6ca394 to 804c274 May 24, 2019

@cdeil

This comment has been minimized.

Copy link
Member Author

commented May 24, 2019

@debaice tested with a ctobssim generated events file. Doesn't work yet:
https://gist.github.com/debaice/681f1d357da26af86ec8cf5ca3eb4ad2

I'll create a test case in gammapy-extra, but I'd prefer to merge now and do this in a follow-up PR next week.

@cdeil

This comment has been minimized.

Copy link
Member Author

commented May 24, 2019

I'm merging this now. If this causes any issues, please shout here and I'll fix tonight.

@cdeil cdeil merged commit 12ab685 into gammapy:master May 24, 2019

4 of 9 checks passed

gammapy.gammapy Build #20190524.12 had test failures
Details
gammapy.gammapy (Test Python35) Test Python35 failed
Details
Codacy/PR Quality Review Hang in there, Codacy is reviewing your Pull request.
Details
Scrutinizer Running
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
gammapy.gammapy (DevDocs) DevDocs succeeded
Details
gammapy.gammapy (Lint) Lint succeeded
Details
gammapy.gammapy (Test Windows35) Test Windows35 succeeded
Details
gammapy.gammapy (Test Windows37) Test Windows37 succeeded
Details

Data automation moved this from To do to Done May 24, 2019

@adonath adonath changed the title Add DataStoreMaker for IACT DL3 indexing Implement DataStoreMaker for IACT DL3 indexing May 27, 2019

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.