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

Improve data and observation handling #457

Merged
merged 1 commit into from Mar 15, 2016
Merged

Improve data and observation handling #457

merged 1 commit into from Mar 15, 2016

Conversation

cdeil
Copy link
Contributor

@cdeil cdeil commented Feb 17, 2016

This pull request adds an Observation class to store the events and IRFs for one observation.

@joleroi @adonath @leajouvin Can you please have a look? What do you think?
(I have to go now ... will write this tomorrow morning.)

@cdeil cdeil added the feature label Feb 17, 2016
@cdeil cdeil self-assigned this Feb 17, 2016
@cdeil cdeil added this to the 0.4 milestone Feb 17, 2016
@cdeil
Copy link
Contributor Author

cdeil commented Feb 17, 2016

My opinion: I would find it convenient to add methods to this class to get at commonly used stuff, e.g. an empty image for the FOV, or an exposure image, or a background image or ... But where't the limit, i.e. what goes in separate functions and do those take Observation objects as input or the separate parts?

Do we want such methods here or as standalone functions that work with obs objects?
"""

def make_spectrum_observation(self, target_pos, on_radius):
Copy link
Contributor

Choose a reason for hiding this comment

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

I usually but classmethods on my classes, i.e. in this case I would have put

SpectrumObservation.from_observation()

But I guess that's a matter of taste. I might be better to have make_object funktions because it's more intuitive from the users' point of view.

Copy link
Member

Choose a reason for hiding this comment

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

👍 to SpectrumObservation.from_observation()

@joleroi
Copy link
Contributor

joleroi commented Feb 18, 2016

👍 for 'easy' stuff like an empty image

For more complicated things I would prefer classes/functions that work with the Observation object (see inline comment)

@cdeil cdeil changed the title Add Observation for one IACT "run" Improve data and observation handling Mar 15, 2016
@cdeil
Copy link
Contributor Author

cdeil commented Mar 15, 2016

@joleroi @adonath @leajouvin - This PR changes the way the DataStore works a bit.

You can now load data like this:

from gammapy.data import DataStore
data_store = DataStore.from_dir('$GAMMAPY_EXTRA/datasets/hess-crab4-hd-hap-prod2')
obs = data_store.obs(obs_id=23523)
events = obs.events
aeff = obs.aeff

It's now also possible to access the different HD PSFs:

psf1 = obs.load(hdu_class='psf_3gauss')
psf2 = obs.load(hdu_class='psf_king')

This uses an HDUIndexTable and HDULocation class under the hood, which is a reflection of this scheme:
http://gamma-astro-data-formats.readthedocs.org/en/latest/data_storage/hdu_index/index.html

I'm still not very happy with this.
Specifically there is duplication of information on the DataStoreObservation and the EventList class of information like e.g. livetime or pointing position.
Further comments here, or issues and pull requests to improve data handling are welcome!
But I'm merging this for now, as I think it's a step in the right direction.

cdeil added a commit that referenced this pull request Mar 15, 2016
Improve data and observation handling
@cdeil cdeil merged commit 4a92be9 into gammapy:master Mar 15, 2016
@joleroi
Copy link
Contributor

joleroi commented Mar 16, 2016

Specifically there is duplication of information on the DataStoreObservation and the EventList class of information like e.g. livetime or pointing position

FYI: In the spectrum analysis class I moved on to use only the ObservationTable for storing meta information (i.e. IMO meta info could be removed from the EventList)

@joleroi
Copy link
Contributor

joleroi commented Mar 16, 2016

obs = data_store.obs(obs_id=23523)

In this case obs.psf will return a default PSF and obs.load(hdu_class='psf_3gauss') will overwrite obs.psf, correct?

@cdeil
Copy link
Contributor Author

cdeil commented Mar 16, 2016

FYI: In the spectrum analysis class I moved on to use only the ObservationTable for storing meta information (i.e. IMO meta info could be removed from the EventList)

I tried removing the live time attribute from the event list:
http://gammapy.readthedocs.org/en/latest/api/gammapy.data.EventList.html#gammapy.data.EventList.observation_live_time_duration
This resulted in an error in tests for your spectral code that I couldn't resolve quickly, so I put it back.
If you could remove this and change your code to maybe take an EventListDataset or a DataStoreObservation object, I think that would be good.
Are we agreed that it's OK to start passing DataStoreObservation objects around in Gammapy, i.e. use it as a high-level container to represent all data associated with an observation?
Or is that too tight coupling of data loading and analysis code?
(Also: EventListDataset could probably be removed, with it's functionality merged into DataStoreObservation, no?)

@cdeil
Copy link
Contributor Author

cdeil commented Mar 16, 2016

In this case obs.psf will return a default PSF and obs.load(hdu_class='psf_3gauss') will overwrite obs.psf, correct?

No. obs.psf will load the PSF that is listed first in the HDU index table, i.e. where HDU_TYPE="psf".
And it is a lazyproperty, i.e. will never change.

obs.load will just load and return an object, and not change the obs or obs.psf property state.

If that causes problems / confusion, we could remove the lazyproperty decorator.
(But then there might be use cases where the same file is read many times during an analysis.)

@joleroi
Copy link
Contributor

joleroi commented Mar 16, 2016

I tried removing the live time attribute from the event list:
http://gammapy.readthedocs.org/en/latest/api/gammapy.data.EventList.html#gammapy.data.EventList.observation_live_time_duration
This resulted in an error in tests for your spectral code that I couldn't resolve quickly, so I put it back.

Seem like the guy who wrote the spectrum analysis code wasn't consistent. I will have a look today how to change the spectrum analysis code in order to deal with a DataStoreObservation but ideologically I definitely think it should be used

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants