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

Change ObservationList class to Observations #1888

Merged
merged 2 commits into from Oct 26, 2018

Conversation

Projects
2 participants
@dcfidalgo
Contributor

dcfidalgo commented Oct 25, 2018

This PR is the first step to implement PIG #1877
It starts to evolve the ObservationList class into the new Observations class.
This PR takes care of the renaming, and introduces a basic version of the Observationsclass that works with the current analysis classes. All tests pass locally (only unrelated fails).

A followup PR would be to introduce a convenience class method .from_index(dir, obs_ids) and to adopt the notebooks accordingly.

There are two more things i wanted further feedback on (@cdeil @registerrier @adonath):

  • we are using obs_list for attributes that hold/return a, now, Observations object. Should we change this name to something like obss or stick to obs_list
  • i saw that there is an SpectrumObservationList class. Should i change this as well to keep Gammapy more consistent overall?

@cdeil cdeil self-assigned this Oct 25, 2018

@cdeil cdeil added the cleanup label Oct 25, 2018

@cdeil cdeil added this to the 0.9 milestone Oct 25, 2018

@cdeil cdeil added this to To do in Data via automation Oct 25, 2018

@cdeil

This comment has been minimized.

Member

cdeil commented Oct 25, 2018

we are using obs_list for attributes that hold/return a, now, Observations object. Should we change this name to something like obss or stick to obs_list

Suggest to use observations as variable / property name for Observations objects, and observation for Observation objects.

i saw that there is an SpectrumObservationList class. Should i change this as well to keep Gammapy more consistent overall?

Suggest to leave this alone and just focus on gammapy.data for the next weeks.

"""
def __init__(self, obs_list=None):
self.obs_list = obs_list or []

This comment has been minimized.

@cdeil

cdeil Oct 25, 2018

Member

Maybe change to self._obs_list to have the as a private implementation detail?
That will force us to offer a good API on the Observations object, instead of accessing and manipulating observations.obs_list from all over the place.

There should be a UserList import at the top which can now be removed.

This comment has been minimized.

@dcfidalgo

dcfidalgo Oct 26, 2018

Contributor

Maybe change to self._obs_list to have the as a private implementation detail?

+1 done

There should be a UserList import at the top which can now be removed.

Done, i did some cleanup in the imports as well

@cdeil

This comment has been minimized.

Member

cdeil commented Oct 26, 2018

Thanks!

@cdeil

cdeil approved these changes Oct 26, 2018

@cdeil cdeil merged commit 23c0d88 into gammapy:master Oct 26, 2018

2 of 4 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Scrutinizer Analysis: 812 new issues, 4 updated code elements – Tests: passed
Details

Data automation moved this from To do to Done Oct 26, 2018

@dcfidalgo dcfidalgo deleted the dcfidalgo:introducing_observations branch Oct 26, 2018

@cdeil cdeil changed the title from Introducing the Observations class to Rename ObservationList class to Observations Nov 23, 2018

@cdeil cdeil changed the title from Rename ObservationList class to Observations to Change ObservationList class to Observations Nov 23, 2018

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