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

Change parametrisation from geom_true to energy_axis_true #2479

Merged
merged 12 commits into from Oct 24, 2019

Conversation

@AtreyeeS
Copy link
Member

AtreyeeS commented Oct 18, 2019

@adonath and @Bultako ! I am having 3 test fails in the Analysis tests, after changing the parametrisation in MapDataset and MapDatasetMaker. Can you please follow up from here?

@Bultako Bultako force-pushed the AtreyeeS:cleanup1 branch from 618e759 to f116b93 Oct 19, 2019
@Bultako

This comment has been minimized.

Copy link
Member

Bultako commented Oct 19, 2019

@AtreyeeS @adonath
I've made the modifs related with the high-level interface, all tests pass locally.
I've had to add IRF config params in the hess.ipynb notebook to fix it.
So default IRF params do not seem to work?

@cdeil cdeil added this to the 0.15 milestone Oct 21, 2019
@cdeil cdeil added the cleanup label Oct 21, 2019
Copy link
Member

adonath left a comment

Thanks @Bultako and @AtreyeeS. I've left a few inline comments...

@@ -33,7 +33,12 @@
MIGRA_AXIS_DEFAULT = MapAxis.from_bounds(
0.2, 5, nbin=48, node_type="edges", name="migra"
)
BINSZ_IRF = 0.2
ENERGY_AXIS_DEFAULT = MapAxis.from_edges(

This comment has been minimized.

Copy link
@adonath

adonath Oct 21, 2019

Member

I think for the true energy axis we should not introduce any default, except for e_true = e_reco, as we already do in many places. The default here (looks like it was copied over from gammapy.spectrum) does not really make sense for a cube analysis. The model evaluation will be much to slow and heavily oversampled for no good reason.

Please remove the default axis here and just uses energy_axis_true = energy_axis_reco as a default, where appropriate.

This comment has been minimized.

Copy link
@AtreyeeS

AtreyeeS Oct 21, 2019

Author Member

Yes, I did indeed copy it over from gammapy.spectrum, and this is clearly a poor choice of binning (which I think we need to take from the IRFs at some point anyways). But I think it might be needed to have a default e_true binning. I remember I was getting very different indices between spectral and map based analysis once, which was solved by changing the e_true. This would be quite non-intuitive for a general user...

@registerrier @bkhelifi do you have an opinion about this?

This comment has been minimized.

Copy link
@adonath

adonath Oct 22, 2019

Member

Yes, the assumption of the 3D model evaluation is that we use a "fine" binning (I think we never clarified, what this actually means, probably >5 bins / decade, but we should document it somewhere). If this is not fulfilled, the user can currently get bad or unexpected results. I think in general we should change to using SpectralModel.integral() in the 3D analysis as well (just as we do for 1D), this should already improve (if not solve...) the discrepancy. Note, that for the SpectrumDatasetMaker I already removed the default for e_true. But we can certainly discuss, whether we re-introduce it in a consistent way and with a plausible choice for 1D as well as 3D.

This comment has been minimized.

Copy link
@registerrier

registerrier Oct 22, 2019

Contributor

I think that the issue is not only the fine binning but the total range.
In theory, the true energy binning is supposed to cover the 0 to + infinity range. It is of course not necessary in practice, but the range has to be larger than the e_reco one to account for spill-over.
This is far from a small approximation in many cases if users define a range close to the actual safe range.
A possibility could be to decide that by default e_true is binned starting from e_reco[0]*0.7 up to e_reco[-1]*1.3. Or even to leave as a possible parameter the leakage fraction (with default at 30% for instance).

@@ -75,25 +93,39 @@ def _cutout_geom(self, geom, observation):
else:
return geom

@lazyproperty
def wcs_irf(self):

This comment has been minimized.

Copy link
@adonath

adonath Oct 21, 2019

Member

Please rename to geom_image_irf...

)

@lazyproperty
def geom_irf(self):

This comment has been minimized.

Copy link
@adonath

adonath Oct 21, 2019

Member

Please rename to geom_exposure_irf

@@ -76,6 +75,17 @@ datasets:
nbin: 73
unit: TeV
interp: log
geom-irf:

This comment has been minimized.

Copy link
@adonath

adonath Oct 21, 2019

Member

@Bultako I think it's clearer if the section in the config file which declares the geometry, directly corresponds to MapDataset.create() like so:

   geom:
        skydir: [83.633, 22.014]
        width: [5, 5]
        binsz: 0.02
        coordsys: CEL
        proj: TAN
        axes:
          - name: energy
            hi_bnd: 10
            lo_bnd: 1
            nbin: 5
            interp: log
            node_type: edges
            unit: TeV
    energy-axis-true:
          - name: energy
            hi_bnd: 10
            lo_bnd: 1
            nbin: 5
            interp: log
            node_type: edges
            unit: TeV
    binsz-irf: 0.2 deg
    margin-irf: 0.5 deg

The additional geom-irf section could be removed.

This comment has been minimized.

Copy link
@Bultako

Bultako Oct 22, 2019

Member

Thanks @adonath

I've addressed your comments in last commit d936eb9 The hess.ipynb notebook still needs the values for true energy as params so to not break.

@adonath

This comment has been minimized.

Copy link
Member

adonath commented Oct 24, 2019

Thanks a lot @AtreyeeS and @Bultako! I've attached one commit with minor clean-up and fixing the light-curve tutorial. Once Travis-CI passes we can merge...

@Bultako I could not reproduce the issue with hess.ipynb. When I removed the info on the true energy axis from the config, it just worked for me. For now I removed to keep the diff in this PR small and see whether CI passes. In general I think we could actually keep the info in the notebook, as might be useful for users as well (but this can be done in a separate PR...)

@Bultako

This comment has been minimized.

Copy link
Member

Bultako commented Oct 24, 2019

I could not reproduce the issue with hess.ipynb.

Yes, me neither, it seems last commits have solved the issue. Fine with me for keeping the notebook unchanged.

@adonath adonath mentioned this pull request Oct 24, 2019
@adonath

This comment has been minimized.

Copy link
Member

adonath commented Oct 24, 2019

Travis is green now, I'll go ahead and merge. Thanks a lot @AtreyeeS and @Bultako.

@adonath adonath merged commit a9eabbe into gammapy:master Oct 24, 2019
8 of 9 checks passed
8 of 9 checks passed
Scrutinizer Errored
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
gammapy.gammapy Build #20191024.5 had test failures
Details
gammapy.gammapy (DevDocs) DevDocs succeeded
Details
gammapy.gammapy (Lint) Lint succeeded
Details
gammapy.gammapy (Test Python36) Test Python36 succeeded
Details
gammapy.gammapy (Test Windows36) Test Windows36 succeeded
Details
gammapy.gammapy (Test Windows37) Test Windows37 succeeded
Details
@Bultako Bultako added this to Done in gammapy.analysis Nov 5, 2019
@adonath adonath changed the title Change parametrisation from `geom_true` to `energy_axis_true` Change parametrisation from geom_true to energy_axis_true Nov 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
5 participants
You can’t perform that action at this time.