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

Angular error regressor #2503

Draft
wants to merge 22 commits into
base: main
Choose a base branch
from
Draft

Angular error regressor #2503

wants to merge 22 commits into from

Conversation

JBernete
Copy link

Adding tools to reconstruct the angular error at the DL2 level. Which will allow the definition of event types based on the angular reconstruction quality.

@kosack
Copy link
Contributor

kosack commented Jan 25, 2024

Can you define "angular error"? It would be useful to explain the use case for this, and what it does in the description. I suspect the name "angular error" may be too generic - there are many angles we reconstruct, which one is this referring to?

@kosack
Copy link
Contributor

kosack commented Jan 25, 2024

This PR also seems to include #2497, maybe it should be parented to that branch?

@JBernete
Copy link
Author

With angular error I mean the angular difference between the true direction and the reconstructed direction of an event. This is useful to define event types in terms of the quality of the reconstruction.

@maxnoe
Copy link
Member

maxnoe commented Jan 25, 2024

This PR also seems to include #2497, maybe it should be parented to that branch?

This PR needs #2497 to create the input features for the machine learning. I think we should first merge 2497 and then update here, not make 2497 bigger by including this branch there.

@kosack
Copy link
Contributor

kosack commented Jan 25, 2024

With angular error I mean the angular difference between the true direction and the reconstructed direction of an event. This is useful to define event types in terms of the quality of the reconstruction.

I suggest naming it "DirectionReconstructionError" or something more clear, since AngularError could refer to other angles. Or maybe better yet "DirectionReconstructionUncertainty" to avoid the class being confused with RuntimeErrors...

Or just "DirectionUncertaintly" if you want something shorter...


result = Table(
{
f"{self.reconstructor_prefix}_ang_distance_uncert": ang_error,
f"{self.reconstructor_prefix}_direction_uncertainty": dir_uncert,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This column name actually needs to stay the same, as it comes from the definitions in ctapipe.containers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could also just rename it to direction_uncertainty, since we have not really used that column up to now. It's technically a data model change, but again, that column was never filled, so shouldn't cause a problem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The stereo combiner fills this column when combining alt/az predictions by disp. But I don't think this was used for anything up until now, so changing it shouldn't be a problem (and direction_uncertainty is a better name imo).

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 this pull request may close these issues.

4 participants