-
Notifications
You must be signed in to change notification settings - Fork 188
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 is_pointlike property on irfs #3404
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3404 +/- ##
=======================================
Coverage 93.55% 93.56%
=======================================
Files 147 147
Lines 18512 18539 +27
=======================================
+ Hits 17319 17346 +27
Misses 1193 1193
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @registerrier, the is a useful addition. I have left some inline comments..
gammapy/irf/core.py
Outdated
axes = MapAxes(axes) | ||
axes.assert_names(self.required_axes) | ||
self._axes = axes | ||
self.data = data | ||
self.unit = unit | ||
self.meta = meta or {} | ||
self._is_pointlike = is_pointlike |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe treat the is_pointlike
info as part of the meta
data? I guess the motivation for the separate argument is to stay independent of the serialisation format (which is good!), however we can introduce arbitrary internal meta data and corresponding keywords as we like. The advantage is that the meta data can be easier copied over to the reduced data as well and we keep the option for a unified data structure of IRF
and Map
in future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could possibly add a table in the docs with "allowed" meta keywords for IRFs and a corresponding description. Like:
Keyword | Description | Type |
---|---|---|
is_pointlike | Whether IRF is pointlike | bool |
safe_energy_range | Recommend safe energy range | tuple |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @registerrier, I left two more minor comments. After those have been addressed, CI should turn green...
@@ -23,6 +23,12 @@ def observations_hess_dl3(): | |||
obs_ids = [23523, 23526] | |||
return datastore.get_observations(obs_ids) | |||
|
|||
@pytest.fixture |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add missing requires_data()
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @registerrier! No further comments from my side...
Description
This pull request adds a
IRF.is_pointlike
property to be able to check whether an IRF is pointlike or not.Dear reviewer