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

Add ObservationFilter select methods #1966

Merged
merged 8 commits into from Dec 20, 2018

Conversation

@dcfidalgo
Copy link
Contributor

@dcfidalgo dcfidalgo commented Dec 5, 2018

This is the first PR of the third scenario of #1877 .
It implements two "select" methods (EventListBase.select_parameter and GTI.select_time) that will be used in the ObservationFilter.

Actually i renamed the EventListBase.select_custom method to EventListBase.select_parameter, because it sounds more intuitive to me.

Apart from the tests to the "select" methods, i also expanded the tests for the ObservationFilter. All tests pass locally.

A follow-up PR would be to introduce a "select_time" method on the DataStoreObservation that automatically adds the necessary filter to the observation. I will also try to update a bit PIG #1877 to further outline the implementation road map.

@dcfidalgo dcfidalgo requested a review from cdeil Dec 5, 2018
@cdeil cdeil removed their request for review Dec 5, 2018
@cdeil
Copy link
Member

@cdeil cdeil commented Dec 5, 2018

@dcfidalgo - Either Axel or Regis will review here and in other Gammapy PRs. I'll join the weekly calls on Friday, where I can give my 2 cents, but not continue with PR detailed review any more.

@dcfidalgo
Copy link
Contributor Author

@dcfidalgo dcfidalgo commented Dec 6, 2018

@cdeil Ok, understood! Unfortunately, tomorrow i won't be able to join the call, but next week for sure.
@adonath @registerrier Does someone want to adopt me? :)

dcfidalgo
@adonath adonath self-assigned this Dec 7, 2018
@adonath adonath added the feature label Dec 7, 2018
@adonath adonath added this to the 0.10 milestone Dec 7, 2018
@adonath adonath self-requested a review Dec 12, 2018
Copy link
Member

@adonath adonath left a comment

Thanks @dcfidalgo! I've added a few minor comments. It would be nice to have a small test or docs example that does a spectral analysis for as short time interval for some test dataset. This should be possible after this PR. But this could be also done in a later follow-up PR...I can file a reminder issue.

@@ -379,22 +379,31 @@ def filter_circular_region(self, region):
mask = np.union1d(mask, temp)
return mask

def select_custom(self, parameter, limits):
"""Select events with respect to a custom parameter
def select_parameter(self, parameter, limits):

This comment has been minimized.

@adonath

adonath Dec 13, 2018
Member

Maybe rename limits to band? So it becomes consistent with select_offset(offset_band=) as well as select_energy(energy_band=)?

This comment has been minimized.

@dcfidalgo

dcfidalgo Dec 17, 2018
Author Contributor

Done

--------
>>> from gammapy.data import EventList
>>> event_list = EventList.read('events.fits')
>>> phase_region = (0.3, 0.5)

This comment has been minimized.

@adonath

adonath Dec 13, 2018
Member

Maybe consider supporting quantities for the limits?

This comment has been minimized.

@dcfidalgo

dcfidalgo Dec 17, 2018
Author Contributor

Done

)
def test_select_parameter(self, parameter, limits):
selected_events = self.events.select_parameter(parameter, limits)
assert all(

This comment has been minimized.

@adonath

adonath Dec 13, 2018
Member

Can you change to the .all() method for numpy arrays? Even if performance is no issue here, it's typically the numpy methods we use...

This comment has been minimized.

@dcfidalgo

dcfidalgo Dec 17, 2018
Author Contributor

Done

events = observation.events
filtered_events = obs_filter.filter_events(events)

assert all(

This comment has been minimized.

@adonath

adonath Dec 13, 2018
Member

See my comment above...

This comment has been minimized.

@dcfidalgo

dcfidalgo Dec 17, 2018
Author Contributor

Done

mask |= self.time_stop <= time_interval[0]
gti_within = self.table[~mask]

# crop the GTIs

This comment has been minimized.

@adonath

adonath Dec 13, 2018
Member

For cropping I find np.clip()a bit easier to read...

This comment has been minimized.

@dcfidalgo

dcfidalgo Dec 17, 2018
Author Contributor

yes, thanks, it reads much better now!

@@ -116,4 +116,20 @@ def select_time(self, time_interval):
gti : `GTI`
Copy of the GTI table with selection applied.
"""
raise NotImplementedError
# get GTIs that fall within the time_interval
mask = self.time_start >= time_interval[1]

This comment has been minimized.

@adonath

adonath Dec 13, 2018
Member

Is this implementation correct? If there are multiple GTIs present and you want want to find those overlapping the selected time interval I'd rather write:

mask = self.time_stop >= time_interval[0]
mask &= self.time_start <= time_interval[1]
self.table[mask]

I just realized, that it's probably equivalent to what you wrote, but confused me a little because you invert the mask at the end...maybe change to the simpler implementation?

This comment has been minimized.

@dcfidalgo

dcfidalgo Dec 17, 2018
Author Contributor

Done

filtered_gti = obs_filter.filter_gti(gti)

# small workaround to test for greater/less than or equal to
abs_diff = np.abs((filtered_gti.time_start - time_filter[0]).sec) # absolute differences in seconds

This comment has been minimized.

@adonath

adonath Dec 13, 2018
Member

I've thought a little about this test case here and I think in the current implementation it's not a useful test. Maybe choose the time filter limits corresponding to the limits of the GTI and just make sure that it's handled correctly?

This comment has been minimized.

@dcfidalgo

dcfidalgo Dec 17, 2018
Author Contributor

Your are right! I extended a bit the tests for the GTI.select_time method, and kept this test rather simple.

@dcfidalgo
Copy link
Contributor Author

@dcfidalgo dcfidalgo commented Dec 14, 2018

@adonath I started to implement the comments and pushed a first commit, but i still want to have a final look at it, so still not ready to merge.
Regarding the docs example of a spectral analysis of a small time interval, i would prefer to do this in a follow-up PR next week.

@dcfidalgo
Copy link
Contributor Author

@dcfidalgo dcfidalgo commented Dec 17, 2018

@adonath I had a final look, and from my side this is ready to merge (Travis fails are unrelated). Thanks again for the comments!

@adonath
Copy link
Member

@adonath adonath commented Dec 20, 2018

@dcfidalgo Thanks, looks good to me as well! Do you still have time for the docs example? If not could you please open a reminder issue?

@adonath adonath merged commit 2de0876 into gammapy:master Dec 20, 2018
4 of 5 checks passed
4 of 5 checks passed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Scrutinizer Analysis: 8 updated code elements – Tests: passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
gammapy.gammapy Build #20181217.1 succeeded
Details
@dcfidalgo dcfidalgo deleted the dcfidalgo:time_and_custom_filter branch Dec 21, 2018
@cdeil cdeil changed the title Implementing select_parameter and select_time used by the ObservationFilter Add ObservationFilter select methods Jan 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants