Improve flux points class #1903
Conversation
| @@ -236,6 +237,58 @@ def drop_ul(self): | |||
| table_drop_ul = self.table[~self.is_ul] | |||
| return self.__class__(table_drop_ul) | |||
|
|
|||
| def _flux_to_dnde(self, e_ref, table, model, pwl_approx): | |||
There was a problem hiding this comment.
I see this argument pwl_approx in addition to model here and in other places.
Can't the same feature already by achieved just by passing a power-law in model and the extra option isn't really useful?
Remove it?
There was a problem hiding this comment.
Yes and no. If you pass in a power-law spectral model the result is the same. If the model is curved, than pwl_approx does a power-law approximation of the model within emin and emax to integrate. If pwl_approx is set to false the model is integrated analytically if possible. I think the pwl_approx option was only added, because it is equivalent to what the Fermi ST do...
There was a problem hiding this comment.
Ah, I guess being able to reproduce what is done in the Fermi catalogs might be a feature worth having built-in. If you keep it, maybe add a test to check for one spectral point in a pulsar cutoff region from 3FGL and 3FHL, to really know what they and we have? And if pwl_approx stays, mention the use case in the docstring?
Otherwise IMO it's unclear why that option exists and when one would want to use it.
There was a problem hiding this comment.
This test already existed. I accidentally deleted it, but added it back in 37905c4. The option is already documented and explained in the public .to_sed_type() method.
gammapy/spectrum/flux_point.py
Outdated
| table["dnde"] = dnde | ||
|
|
||
| if "flux_err" in table.colnames: | ||
| # TODO: implement better error handling, e.g. MC based method |
There was a problem hiding this comment.
Remove the TODO?
Especially the suggestion to introduce MC sampling here seems unlikely to me.
|
|
||
| @pytest.fixture(scope="session") | ||
| def flux_points_dnde(): | ||
| e_ref = [np.sqrt(10), np.sqrt(10 * 100)] * u.TeV |
There was a problem hiding this comment.
Could be simplified a bit to not mess around with sqrt, by just using powers of ten, e.g. a bin from 10 TeV to 1000 TeV with 100 TeV at the center.
Is the second point really useful or would reducing to one basically test the same thing that the methods are OK?
Add asserts below on the transformed errors that are also computed in those methods?
gammapy/catalog/fermi.py
Outdated
| elif column == "is_ul": | ||
| continue | ||
| else: | ||
| table[column].format = ".3" |
There was a problem hiding this comment.
This will raise a ValueError if someone has e.g. an extra bool column publish_point or whatever in the table.
repr is used for debugging, so having repr error out with a ValueError when someone has a weird table isn't nice.
Generally I think our philosophy everywhere in Gammapy it so access some keys in meta and columns in table, and to just ignore the rest.
So even if it's a bit more work to type, I'd suggest to have an explicit list of columns to format and do nothing with the rest.
There might be a possibility to reduce a bit of code duplication and have a helper function that's used in gammapy.catalog as well. I didn't look in detail, but there we have this:
$ ack '\.format = ' gammapy/catalog
gammapy/catalog/gammacat.py
389: table["e_ref"].format = ".3f"
392: table[_].format = ".3e"
gammapy/catalog/fermi.py
390: table["sqrt_TS"].format = ".1f"
391: table["e_ref"].format = ".1f"
393: table[_].format = ".3"
883: table["sqrt_ts"].format = ".1f"
884: table["e_ref"].format = ".1f"
886: table[_].format = ".3"
gammapy/catalog/hess.py
418: flux_points[_].format = ".3f"
421: flux_points[_].format = ".3e"
``
There was a problem hiding this comment.
I decided to add a FluxPoints.table_formatted property and only applied the formatting to a certain set of columns. Others remain unchanged...
This PR addresses the issues #1034 as well as #918. It implements the following changes:
dnde->e2dndeand back.sed_typeoption fromFluxPoints.plot(), to avoid confusion.