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

fix: also add tracks that failed smoothing to failed_tracks #1741

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wdconinc
Copy link
Contributor

Briefly, what does this PR introduce?

Since #1677 we remove tracks that failed extrapolation. We don't do that with tracks that failed smoothing. This PR also applies the same treatment to tracks that failed smoothing.

What kind of change does this PR introduce?

  • Bug fix (issue #__)
  • New feature (issue #__)
  • Documentation update
  • Other: __

Please check if this PR fulfills the following:

  • Tests for the changes have been added
  • Documentation has been added / updated
  • Changes have been communicated to collaborators

Does this PR introduce breaking changes? What changes might users need to make to their code?

No.

Does this PR change default behavior?

No.

Sorry, something went wrong.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@wdconinc wdconinc requested a review from veprbl February 21, 2025 21:09
@github-actions github-actions bot added the topic: tracking Relates to tracking reconstruction label Feb 21, 2025
@veprbl
Copy link
Member

veprbl commented Mar 16, 2025

I don't think we should be "hiding" tracks that failed smothing without a clear reason. If the tracks constituted by background, or have unreasonable parameters estimated, then we could reconsider. Would you agree?

@wdconinc
Copy link
Contributor Author

I thought that ship had sailed when we decided to hide tracks that fail extrapolation? How is smoothing different?

@veprbl
Copy link
Member

veprbl commented Mar 16, 2025

I thought that ship had sailed when we decided to hide tracks that fail extrapolation? How is smoothing different?

Tracks that fail extrapolation do not have a usable set of parameters, so we can't store them.

@wdconinc
Copy link
Contributor Author

I thought that ship had sailed when we decided to hide tracks that fail extrapolation? How is smoothing different?

Tracks that fail extrapolation do not have a usable set of parameters, so we can't store them.

But we don't even attempt extrapolation when smoothing fails. If we want to allow tracks that failed smoothing, then we should still attempt extrapolation.

@veprbl
Copy link
Member

veprbl commented Mar 16, 2025

I thought that ship had sailed when we decided to hide tracks that fail extrapolation? How is smoothing different?

Tracks that fail extrapolation do not have a usable set of parameters, so we can't store them.

But we don't even attempt extrapolation when smoothing fails. If we want to allow tracks that failed smoothing, then we should still attempt extrapolation.

Right, so in the current setup, smoothing has never ever failed, because if it would, that'd cause a segfault. If we apply your suggested change, if will be allowed to fail without interrupting anything.

@veprbl
Copy link
Member

veprbl commented Mar 16, 2025

Should we pivot towards making this into a "fatal" exception?

@wdconinc
Copy link
Contributor Author

Should we pivot towards making this into a "fatal" exception?

Define 'fatal'.

  • It indicates something is fundamentally wrong with the geometry conversion to ACTS, so we should throw an exception and just fail the entire run.
  • It is fatal for this track only, and we skip it entirely.

Sorry, something went wrong.

@veprbl
Copy link
Member

veprbl commented Mar 16, 2025

Fatal as in this was never expected to happen and user is forced to report/confront this.

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

Successfully merging this pull request may close these issues.

None yet

2 participants