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

Rename obs_list to observations #1895

Merged
merged 4 commits into from Nov 2, 2018
Merged

Conversation

@dcfidalgo
Copy link
Contributor

@dcfidalgo dcfidalgo commented Oct 29, 2018

As suggested in #1888, i renamed all the obs and obs_list class attributes in Gammapy to observation and observations, respectively.
This naturally affects a LOT of files, but i think this change is worth it/necessary.
There are two commits, the first one tackles obs -> observation, the second one obs_list -> observations.

One additional change:

  • The SpectrumExtractor class had an obs_list and observations attribute, referring to an ObservationList and a SpectrumObservationList, respectively. I changed it to obs_list -> observations and observations -> spectrum_observations. In general i think it is very confusing to have a SpectrumObservation alongside the observation classes, i would propose to rename it to maybe SpectrumDataset?

All tests and notebooks pass locally (pycharm->refactor and ack were my friends).
I think this renaming makes Gammapy a bit more "explicit" (maybe even R-rated ...) and helps new developers to not get confused by obs_list (now observations) not being a python list.

@adonath
Copy link
Member

@adonath adonath commented Oct 29, 2018

One minor comment from "outside"(because I haven't been involved in the discussions concerning this PR so far):

We don't have a consistent naming scheme for methods and attributes / properties yet, but wouldn't it make sense to rename the DataStore.observations() method to DataStore.get_observations()? To distinguish it from the classes, where .observations is an attribute and not a method?

Loading

@dcfidalgo
Copy link
Contributor Author

@dcfidalgo dcfidalgo commented Oct 29, 2018

I'm +1 for changing to DataStore.get_observation() and DataStore.get_observations(), but no strong opinion. Or even remove/make private the get_observation method to make the API more compact.

Loading

@cdeil cdeil added this to the 0.9 milestone Oct 29, 2018
@cdeil cdeil added this to To do in gammapy.data via automation Oct 29, 2018
@cdeil cdeil requested a review from registerrier Oct 29, 2018
@cdeil
Copy link
Member

@cdeil cdeil commented Oct 29, 2018

I see, so this will break all user scripts that contain data_store.obs or data_store.obs_list. OK, fine, I guess it can't really be avoided.
I don't have a particular preference for either observation / observations versus get_observation, get_observations. Yes, maybe the get_ is more explicit that it's a method and not a property, so OK to go that way if you like.

@registerrier - Please comment here also. This PR should go in quickly, to avoid merge conflict or blocking other edits.

Loading

Copy link
Contributor

@registerrier registerrier left a comment

I would make such a change that will affect basically all scripts and notebooks only once naming pattern is clear. At the moment, I am afraid that:

  • DataStore.observation and DataStore.observations (or .get_observation()) are way too similar names. This is confusing and will result in numerous errors.
  • I would not touch SpectrumObservation before things are at least discussed regarding datasets. Or it will likely have to be renamed/changed once again.

Loading

@cdeil
Copy link
Member

@cdeil cdeil commented Oct 29, 2018

@registerrier - What do you propose?

Please state an alternative proposal and then we'll discus and decide in the next call.

I thought we already discussed Observations vs ObservationCollection in Madrid and there was a small preference for the shorter Observations? Do you want ObservationCollection or something else?

Loading

@registerrier
Copy link
Contributor

@registerrier registerrier commented Oct 30, 2018

Well Observations as a class name is probably fine. I am not suggesting to change it.
At the moment DataStore is really an important element for the user interface. The distinction between DataStore.observation and DatStore.observations is error prone.
A different proposal could be not to change the DataStore.obs interface and all the obs variables in the code and to simply replace DataStore.obs_list() with DataStore.get_observations()

Loading

@adonath
Copy link
Member

@adonath adonath commented Oct 30, 2018

@registerrier As far a I can see the danger of confusion of observation and observations is only given with the DataStore object. In all other cases it is either a single observation (such as MapMakerObs.observation) or many observations (such as ReflectedRegionsBackgroundEstimator.observations), where it's clear from the context what it is...I don't see a strong argument here. I agree it is annoying for users, if we change the API, but I think it's for the better...and we will converge to a stable API for Gammapy 1.0.

I'm fine with this change in general. I slightly prefer the get_observations() name for the methods on the DataStore to distinguish them from class attributes.

Loading

@dcfidalgo
Copy link
Contributor Author

@dcfidalgo dcfidalgo commented Oct 30, 2018

Thank you all for your feedback!

@registerrier - Would it be ok for you if i revert to DataStore.observation -> DataStore.obs and make the change proposed by @adonath (DataStore.observations -> DataStore.get_observations) ?

My original idea was also that the user only interacts with the Observations class, so a user script would always start something like this:

observations = Observations.from_index(dir)
# or
observations = Observations.from_xml(file)

my_observations = observations.select_observations(obs_id)
# or
my_observations = observations.select_time(time_interval)
# or
my_observations = observations.select_region(region)

so no interaction with the DataStore object. I am also not sure if we actually need to expose DataStore.obs, since the same is simply achieved by DataStore.get_observations([obs_id])[0]

Loading

@adonath
Copy link
Member

@adonath adonath commented Oct 30, 2018

@dcfidalgo I think that's what @registerrier already suggested. I'm also fine with with keeping DataStore.obs for now (backwards compatibility at no cost...and remove it in a later release) but never use it in any of our notebooks / docs and only expose the new .get_observations() to the users.

Loading

@cdeil
Copy link
Member

@cdeil cdeil commented Oct 30, 2018

I think it would have been better to do the change to consistency and API break for obs and obs_list at the same time. But OK, then let's leave obs alone. I'd still suggest using observation consistently for variable names in new code, instead of the current mix of obs and observation.

I'm -1 to change use of obs from examples like here for now:
https://docs.gammapy.org/0.8/getting-started.html

This looks a bit silly to me, an API to access a given observation seems normal:

observation = data_store.get_observations([obs_id])[0]

We could do the high-level example changed then once Observations has been developed a bit and grown a nice API, i.e. adapt the high-level docs when you change from DataStore -> Observations. Then the discussion on that method name is irrelavant.

Loading

@dcfidalgo dcfidalgo force-pushed the renaming_obs_list branch from 3859eeb to 3de3466 Oct 30, 2018
cdeil
cdeil approved these changes Oct 31, 2018
Copy link
Member

@cdeil cdeil left a comment

@dcfidalgo - I see you already implemented the changes as discussed a few days ago. Please always leave a comment to let us know when we should review again. This is ready to be merged, no?

@registerrier and @adonath - Please do a final review now.

Loading

Copy link
Contributor

@registerrier registerrier left a comment

Thanks!

Loading

Copy link
Member

@adonath adonath left a comment

I have no further comments. Thanks @dcfidalgo!

Loading

@dcfidalgo
Copy link
Contributor Author

@dcfidalgo dcfidalgo commented Nov 1, 2018

Loading

@adonath
Copy link
Member

@adonath adonath commented Nov 2, 2018

Travis-CI fails are unrelated. I'll merge this PR now.

Loading

@adonath adonath merged commit 7337cee into gammapy:master Nov 2, 2018
3 of 4 checks passed
Loading
gammapy.data automation moved this from To do to Done Nov 2, 2018
@dcfidalgo dcfidalgo deleted the renaming_obs_list branch Nov 7, 2018
@cdeil cdeil changed the title Renaming obs and obs_list attributes Rename obs_list to observations Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
gammapy.data
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants