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 new observation container class #1630

Merged
merged 2 commits into from Aug 4, 2018

Conversation

Projects
2 participants
@dcfidalgo
Contributor

dcfidalgo commented Aug 2, 2018

This PR introduces a new observation container class: ObservationCTA (we want to keep the name Observation free for a possible more generic observation container class in the future that supports other instruments, Fermi/Hawc, as well).

The main goal is to break the dependency of the observation container on the DataStore object. Like this we can create an observation from scratch (programmatically) and we are more flexible for other input formats (for example, the xml format of ctools, http://cta.irap.omp.eu/ctools/users/tutorials/howto/iact/analysis.html?highlight=xml#observation-definition-xml-file).

For this we basically copy the old DataStoreObservation replacing all its properties/lazyproperties with class attributes, that are taken on init.

Regarding the name, ObservationIACT vs ObservationCTA:
@cdeil had a slight preference towards CTA. Personally i do not have a favorite, other opinions are more than welcome.

Follow-up PRs are planed:

  • introduce a method that creates an ObservationCTA from a DataStore object
  • make DataStoreObservation a child class of ObservaionCTA to avoid code duplication, maybe make DataStoreObservation a light version of the current implementation
  • rename DataStoreObservation to either DataStoreObservationCTA or DataStoreObservationIACT for consistency

Looking forward to your thoughts @cdeil @registerrier @adonath and all

@cdeil

@dcfidalgo - Thanks!

This could go in any time, since it's unused. Let me know if you still have time to work on this, or if we should just merge and then this continues at some later time.

If you want to work on this, my suggestions would be (in order of priority):

  • make it more minimal. E.g. pointing_zen and pointing_altaz can be removed, tstart and tstop also because it's duplicate info wrt GTI (and if you kept them, all times should be Time objects). Also the target_radec can go IMO. It's easier to start with something minimal and add to it, than to start with many things and take things away or change them later.
  • Next step would already be to add the DataStoreObservation.to_observation_cta method with a test case
  • Or it could be to add a test case where you read these things and create such an object from scratch, to test __init__ and also test __str__.
  • At this point I would introduce the base class. Keep the names ObservationCTA and DataStoreObservation, and introduce an ObservationBase and copy the __str__ method there, to avoid the code duplication

Those are things that you could do here, or just leave to a future PR.

Then the next step would be to try and pass one such obs to a Gammapy analysis and see if it works; that's for sure for a future PR.

@cdeil cdeil self-assigned this Aug 2, 2018

@cdeil cdeil added the feature label Aug 2, 2018

@cdeil cdeil added this to To do in Data via automation Aug 2, 2018

@cdeil cdeil added this to the 0.8 milestone Aug 2, 2018

@dcfidalgo

This comment has been minimized.

Show comment
Hide comment
@dcfidalgo

dcfidalgo Aug 3, 2018

Contributor

@cdeil Thanks for the comments!

make it more minimal. E.g. pointing_zen and pointing_altaz can be removed, tstart and tstop also because it's duplicate info wrt GTI (and if you kept them, all times should be Time objects). Also the target_radec can go IMO. It's easier to start with something minimal and add to it, than to start with many things and take things away or change them later.

Done, i pushed a commit in which i removed all attributes that are currently not used in Gammapy.

Next week i will still have some limited time in the afternoon to work on this, the week after i will go on holidays. Maybe we could merge this PR just in case, and i will try to open another one next week where i introduce a method to create an ObservationCTA + test.

Contributor

dcfidalgo commented Aug 3, 2018

@cdeil Thanks for the comments!

make it more minimal. E.g. pointing_zen and pointing_altaz can be removed, tstart and tstop also because it's duplicate info wrt GTI (and if you kept them, all times should be Time objects). Also the target_radec can go IMO. It's easier to start with something minimal and add to it, than to start with many things and take things away or change them later.

Done, i pushed a commit in which i removed all attributes that are currently not used in Gammapy.

Next week i will still have some limited time in the afternoon to work on this, the week after i will go on holidays. Maybe we could merge this PR just in case, and i will try to open another one next week where i introduce a method to create an ObservationCTA + test.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Aug 4, 2018

Member

@dcfidalgo - Thanks!

Member

cdeil commented Aug 4, 2018

@dcfidalgo - Thanks!

@cdeil cdeil merged commit 872d5a4 into gammapy:master Aug 4, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

Data automation moved this from To do to Done Aug 4, 2018

@dcfidalgo dcfidalgo deleted the dcfidalgo:new_observation_class branch Aug 6, 2018

@cdeil cdeil changed the title from Introducing new observation container class to Add new observation container class Aug 15, 2018

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