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

FluxPointsEstimator fails if no edisp is set #5274

Closed
AtreyeeS opened this issue May 20, 2024 · 3 comments · Fixed by #5342
Closed

FluxPointsEstimator fails if no edisp is set #5274

AtreyeeS opened this issue May 20, 2024 · 3 comments · Fixed by #5342
Assignees
Labels
bug cleanup coding sprint To be tackled during coding sprint
Milestone

Comments

@AtreyeeS
Copy link
Member

Gammapy version
present dev

Bug description

Running FluxPointsEstimator on a dataset without an edisp gives a very confusing error message ValueError: Can only stack equivalent maps or cutout of the same map.

To Reproduce

Set edisp=None for any dataset (with e_true=e_reco) and proceed with flux points estimation in multiple energy bands (normal use case).

Expected behaviour
The analysis fails (as expected) during the fitting stage if energy_true has a different binning from the energy axis with a more understandable error

File ~/Gammapy-dev/gammapy/gammapy/datasets/evaluator.py:245, in MapEvaluator._edisp_diagonal(self)
    242 @lazyproperty
    243 def _edisp_diagonal(self):
    244     return EDispKernel.from_diagonal_response(
--> 245         energy_axis_true=self.edisp.axes["energy_true"],
    246         energy_axis=self.edisp.axes["energy"],
    247     )

AttributeError: 'NoneType' object has no attribute 'axes'

However, if the energy_axis and the energy_axis_true have same binning, the fit proceeds fine and then the very confusing error is raised during flux points estimation.

  1. Either the flux estimator (and also the fit) should fail with a decent error message consistently for no edisp OR
  2. the diagonal edisp can be adapted with the energy_axis here
    def _edisp_diagonal(self):

    to use the dataset energy axis.
@AtreyeeS AtreyeeS added this to To do in gammapy.modeling via automation May 21, 2024
@AtreyeeS AtreyeeS added this to To do in gammapy.estimators via automation May 21, 2024
@AtreyeeS AtreyeeS added this to the 1.3 milestone May 21, 2024
@QRemy QRemy added the coding sprint To be tackled during coding sprint label Jun 6, 2024
@fabiopintore fabiopintore self-assigned this Jun 11, 2024
@fabiopintore
Copy link
Contributor

fabiopintore commented Jun 11, 2024

Thanks @AtreyeeS for pointing this out.
I've also note that the same issue happens when running Fit on a dataset without edisp. I think that a possibility should be to operate in Dataset, or even in the Maker, and allow for a default diagonal edisp when the latter is not present. Any opinion about this?

@AtreyeeS
Copy link
Member Author

If you have a dataset with energy_true = energy, then it works for the fitting by silently creating a diagonal edisp matrix and fails in the flux estimation.

I propose we either always create a diagonal edisp (with a correct logging info), or we always fail.

@fabiopintore
Copy link
Contributor

Yes, correct. But if you create the dataset without selecting edisp, Fit crashes. I think that we your first option is the most suitable, i.e. it always creates a diagonal matrix and gives an info on that. I wouldn't got with the failing because someone could want to estimate or fit the dataset without including edisp.

gammapy.modeling automation moved this from To do to Done Jul 5, 2024
gammapy.estimators automation moved this from To do to Done Jul 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment