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

Interpolation of the energy dispersion matrix #94

Closed
YoshikiOhtani opened this issue Sep 9, 2022 · 2 comments · Fixed by #95
Closed

Interpolation of the energy dispersion matrix #94

YoshikiOhtani opened this issue Sep 9, 2022 · 2 comments · Fixed by #95
Assignees

Comments

@YoshikiOhtani
Copy link
Collaborator

YoshikiOhtani commented Sep 9, 2022

I found that the interpolated energy dispersion matrix with the "nearest" method is somehow changed from the original matrix as shown in the attached plots. It seems to me that there is an issue in the pyirf (v0.6.0) interpolation function pyirf.interpolation.interpolate_energy_dispersion, where the interpolated matrix is renormalized:
https://github.com/cta-observatory/pyirf/blob/3f9cfba672c8a25eb5094d25acf411c6b2104112/pyirf/interpolation.py#L92

According to the comment it tries to renormalize along the migration axis, but I think the specified axis is wrong. It seems the function assumes that the returned matrix matrix_interp from the astropy.interpolate.griddata is the shape (n_energy_bins, n_migration_bins, n_fov_offset_bins), but when I checked it by myself it is returned with an extra axis at the beginning which is originally used for storing multiple matrixes of all the grid points. So the current normalization with the axis 1 must be done along the energy axis, resulting the matrix different from the original one.

edisp_before_interp
edisp_after_interp

The wrongly interpolated energy dispersion clearly affects the reconstruction of the Crab spectrum (I will add the plots later here) as shown in the attached plots. Since the interpolation method is already updated in pyirf v0.7.0, it does not make sense to open an issue in the pyirf repository. One solution is to implement a custom interpolation method in our pipeline which normalizes along the migration axis, until we update pyirf to newer versions. Please let me know what you think @jsitarek.

crab_sed
crab_sed_interp

@jsitarek
Copy link
Collaborator

HI @YoshikiOhtani

sorry for a late feedback.
Indeed you are right, the old code was wrong. The new code also changed the API, so I think your proposed solution is the best: we keep with the reimplemented function in the code, and refactor it once again to a new function when we update the pyirf version.
We can close this issue when #91 is merged

@YoshikiOhtani
Copy link
Collaborator Author

Hi @jsitarek, thanks for your checks. I actually opened another pull request #95 to fix this issue since it is a bug and not related to the refactoring #91, so let me merge #95 to the master branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants