Cleanup FluxMaps a little bit #3275
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3275 +/- ##
==========================================
- Coverage 93.81% 93.74% -0.07%
==========================================
Files 144 144
Lines 17754 17738 -16
==========================================
- Hits 16656 16629 -27
- Misses 1098 1109 +11
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thanks @adonath.
My main concern is the decision to remove the flexibility to support other maps than some specified ones.
This means each time we adapt some estimator or introduce some functionality we have to add the new maps in the list of supported maps. We will introduce some for incompatibility with that. If there is no strong gain to introduce this limited list of supported map, I would not do that.
| return self.energy_axis.edges[1:] | ||
|
|
||
| @property | ||
| def ts(self): |
There was a problem hiding this comment.
Do we need to add a check for the existence of the ts entry in self.data or can we assume it will always be there?
Note that if we use this for FluxPoints as well this will no longer be a Map object.
|
|
||
| @property | ||
| def ts(self): | ||
| if not "ts" in self.data: |
There was a problem hiding this comment.
Here there is a test.
| str_ += "\n\t"+"\n\t".join(str(self.norm.geom).split("\n")[2:]) | ||
| str_ += "-" * len(self.__class__.__name__) | ||
| str_ += "\n\n" | ||
| str_ += "\t" + "\t\n".join(str(self.norm.geom).split("\n")[:1]) |
There was a problem hiding this comment.
Maybe use self.geom if it exists now.
|
|
||
| rows = [] | ||
| for idx, energy in enumerate(self.energy_ref): |
There was a problem hiding this comment.
If you remove this, you won't have the various energy columns in the final Table, right?
| factor = reference_model.spectral_model.energy_flux(e_edges[:-1], e_edges[1:]) | ||
| elif sed_type == "e2dnde": | ||
| factor = e_ref ** 2 * ref_dnde | ||
| with np.errstate(invalid="ignore", divide="ignore"): |
There was a problem hiding this comment.
The reference_flux method is an elegant solution.
BTW, should we protect the class to make sure that no model is passed which carries the flux on the spatial_model. It shouldn't be the case most of the time, but who knows...
There was a problem hiding this comment.
Yes, I guess the alternative solution is to evaluate the reference flux on the SkyModel and integrate over the spatial part, but that's probably too much trouble for almost no benefit....I agree that using a cube template as reference model could be just forbidden.
gammapy/estimators/flux_map.py
Outdated
|
|
||
| # We add the remaining maps | ||
| for key in maps.keys() - (REQUIRED_MAPS[sed_type] + OPTIONAL_MAPS[sed_type]): | ||
| data[key] = maps[key] | ||
| for key in COMMON_MAPS: |
There was a problem hiding this comment.
The issue with this is that you can get a different dict in output.
Also we don't support other possible entries.
gammapy/estimators/flux_map.py
Outdated
| } | ||
|
|
||
| COMMON_MAPS = ["ts", "sqrt_ts"] |
There was a problem hiding this comment.
How about other maps: counts, background, excess.
ExcessMapEstimator could be adapted to return flux maps as well but we would need other diagnostics maps in that case.
Shouldn't we leave the flexibility of using any other map?
There was a problem hiding this comment.
This was the main question I had as well. In general I would prefer to declare somehow which other maps are allowed as well, but this requires to define first which those are. I think defining and documenting the additional quantities e.g. a ExcessMapEstiamtor returns is a good thing. And this requires also clarifying the terms we use. E.g. especially in the context of the ExcessMapEstimator this is not 100% clear to me yet. E.g. currently we return counts and excess and background, while in the context of an Estimator I would rather refer to npred, npred_signal and npred_background, even if it is the same by definition in the Li&Ma case.
As a first start I could open an PR extending the documentation here https://docs.gammapy.org/dev/estimators/index.html, for quantities we are currently missing.
There was a problem hiding this comment.
Following #3278 , I added npred, npred_excess and npred_null to the maps. However it might still be incomplete. I think the next step is to check the result quantities of the current estimators and make a PR to unify it and extend for missing quantities. I'm completely open to allow for any other additional quantity, however for this PR I propose to just stick with the defined and documented ones.
adonath
force-pushed
the
cleanup_flux_maps
branch
from
4395bd8
to
2275a85
Compare
|
@registerrier I'll go ahead and merge this PR. Then we can prepare a second one to unify and add missing quantities to the current Estimators... |
Description
This pull request cleans up the
FluxMapsobject a little bit.Dear reviewer
@registerrier Previously the code allowed any arbitrary extra maps for I/O. I wasn't sure whether this is a good idea or not. In this PR I'm introducing a
COMMON_MAPSvariable that declare the allowed extra quantities. In general this possibly includes a few other such asis_ulorniter.