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

Telescope / NominalFrame for reconstruction / visualization #1315

Closed
4 tasks done
maxnoe opened this issue May 7, 2020 · 6 comments
Closed
4 tasks done

Telescope / NominalFrame for reconstruction / visualization #1315

maxnoe opened this issue May 7, 2020 · 6 comments
Milestone

Comments

@maxnoe
Copy link
Member

maxnoe commented May 7, 2020

This is a meta issue for open issues related to improving calculation of image parameters,
reconstruction and plotting.

@maxnoe maxnoe added this to the v0.9.0 milestone May 7, 2020
@HealthyPear
Copy link
Member

I am biased since I really need to fix this, but I propose to start from the first one :)

For protopipe, there are 2 different, but related aspects:

  • use the effective focal length to radially scale the camera (I think now in ctapipe there is an option to choose, but the values are not in the simtel file, rather in this table),
  • change the order of the operations in a way that the Hillas parameters have already dimension of degrees before the direction reconstruction is performed.

The second aspect is a mixture of two things:

  • the reconstructor must change (protopipe uses by default ctapipe.reco.HillasReconstructor) in order to not make this transformation
  • the transformation itself should happen once at the beginning (no information from the image itself is needed), and this is probably more a pipeline issue
    If someone remembers, I already showed a proposal for a way to do this here (the notebook titled test_camera_shrinking).

There I transform to TelescopeFrame, but I imagine that one can do the same for Nominal.
What matters for the final result is the use of the right focal length and the order of the operations.

The only problem for me is that for some reason (either it is supposed to be like this or I made a mistake, but I didn't find it yet) the performance (very simple, just the theta2 distribution right out from ctapipe.reco.HillasReconstructor) using the effective focal length seems bad compared than the one calculated using the focal length currently used in ctapipe (nominal).

So I would propose to start from there (I can move this into a dedicated issue or open a PR right away to test this if we agree).

@maxnoe
Copy link
Member Author

maxnoe commented May 7, 2020

@HealthyPear actually, I think the discussion on which focal length to use for this transformation is a completely different one.

For this issue here, let's assume we have the right one and discuss the necessary steps to calculate this stuff in the respective coordinate frames.

Which focal length to put in is also a very simple change, unlike the general restructuring that's needed to make this happen.

@HealthyPear
Copy link
Member

HealthyPear commented May 7, 2020

Sure! As I said it is already possible to choose which focal length, and then it's just a matter of initializing CameraFrame with the correct value.

Regarding the general structuring: I think we need just to decouple the changes related to the library (ctapipe) from those related to the order of the operations (the pipeline in general).

I think that from the point of view of ctapipe, one thing that needs to be modified for sure is the reconstructor (like I did in the notebook).
In the case of ctapipe.reco.HillasReconstructor for example, by default, is expecting an input from a CameraFrame.

What I did was to move the transformation outside, in-between image cleaning and parametrization.
The only thing that ctapipe should be concerned with, I think, is the fact that the transformation is not done anymore by the reconstructor.

The rest is more a matter of pipeline, I guess...

@HealthyPear
Copy link
Member

HealthyPear commented Jul 7, 2020

An update on this

I have looked into PR #1191 and used that code to make a new test.
I focused on the parametrization and direction reconstruction (this part is new with respect that PR, so it should be put to a new one).
I didn't have time to actually solve the visualization problems.

I attach here a PDF (the upload of a jupyter notebook seems to not be supported, unfortunately...).
optical_aberrations_correction_CTA-MARS_test.pdf

It seems to work at least for point-source simulations, it shouldn't be difficult to extend it also to divergent.

Since this is my best solution at the moment, I added it in a test branch of protopipe to create some parameter distributions in degrees to compare with @moralejo ; if it turns out to be correct I can open the PR.

@HealthyPear
Copy link
Member

UPDATE:

What I was talking about is now working and ready for review in PR #1408.
It solves more or less all the points listed at the beginning, perhaps some things can be refined later on.

@maxnoe maxnoe modified the milestones: next major release, v0.12.0 Jul 2, 2021
@maxnoe maxnoe modified the milestones: v0.12.0, v0.13.0 Dec 6, 2021
@maxnoe
Copy link
Member Author

maxnoe commented Dec 6, 2021

Done

@maxnoe maxnoe closed this as completed Dec 6, 2021
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

No branches or pull requests

2 participants