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

Improve flux points class #1903

Merged
merged 9 commits into from Nov 7, 2018
Merged

Conversation

@adonath
Copy link
Member

@adonath adonath commented Nov 6, 2018

This PR addresses the issues #1034 as well as #918. It implements the following changes:

  • Remove the internal SED type conversion of flux points from the Fermi catalog objects.
  • Implement the possibility to convert from dnde -> e2dnde and back.
  • Add unit tests for the sed type conversions.
  • Remove the sed_type option from FluxPoints.plot(), to avoid confusion.
@adonath adonath self-assigned this Nov 6, 2018
@adonath adonath added the cleanup label Nov 6, 2018
@adonath adonath added this to the 0.9 milestone Nov 6, 2018
Copy link
Member

@cdeil cdeil left a comment

@adonath - Some comments inline.

@@ -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):

This comment has been minimized.

@cdeil

cdeil Nov 7, 2018
Member

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?

This comment has been minimized.

@adonath

adonath Nov 7, 2018
Author Member

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...

This comment has been minimized.

@cdeil

cdeil Nov 7, 2018
Member

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.

This comment has been minimized.

@adonath

adonath Nov 7, 2018
Author Member

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.

table["dnde"] = dnde

if "flux_err" in table.colnames:
# TODO: implement better error handling, e.g. MC based method

This comment has been minimized.

@cdeil

cdeil Nov 7, 2018
Member

Remove the TODO?
Especially the suggestion to introduce MC sampling here seems unlikely to me.

This comment has been minimized.

@adonath

adonath Nov 7, 2018
Author Member

Done


@pytest.fixture(scope="session")
def flux_points_dnde():
e_ref = [np.sqrt(10), np.sqrt(10 * 100)] * u.TeV

This comment has been minimized.

@cdeil

cdeil Nov 7, 2018
Member

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?

elif column == "is_ul":
continue
else:
table[column].format = ".3"

This comment has been minimized.

@cdeil

cdeil Nov 7, 2018
Member

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"
``

This comment has been minimized.

@adonath

adonath Nov 7, 2018
Author Member

I decided to add a FluxPoints.table_formatted property and only applied the formatting to a certain set of columns. Others remain unchanged...

@cdeil cdeil added this to To do in gammapy.makers via automation Nov 7, 2018
@adonath adonath merged commit d7d3eb2 into gammapy:master Nov 7, 2018
3 of 4 checks passed
3 of 4 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: 380 new issues, 14 updated code elements – Tests: passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
gammapy.makers automation moved this from To do to Done Nov 7, 2018
@adonath adonath deleted the adonath:improve_flux_points_class branch Nov 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.