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

ref: Remove code duplication from helix intersectors #873

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

niermann999
Copy link
Contributor

Removes the old Newton-Raphson algorithm, which is only used in the jacobian and covaraince transport integration tests, and makes the rt_safe algorithm from numerical recipes the standard in all tests

@niermann999 niermann999 added refactor refactoring the current codes priority: low Low priority labels Oct 29, 2024
@beomki-yeo
Copy link
Collaborator

Please leave this as it is for now. I have not been convinced why the extension you made is required

@niermann999
Copy link
Contributor Author

niermann999 commented Oct 31, 2024

I am not saying it is required, I am saying it is an improvement. My argument is, that it is worthwhile using rt_safe even if not a single additional intersection is recovered:

  • greater conceptual error tolerance (see. Numerical Recipes) => more future-prove for new detector setups/shapes
  • more information about reason for failure (the bracketing provides information that is not present in pure Newton), and hence easier to debug problems. I also improved on the error reporting in this PR
  • bracketing allows for early rejection of intersections that lie outside of the detector space
  • if a Newton step converges slower than a bisection step, take bisection

The last two points might make the it more efficient, but the first two points are really why I would choose this algorithm over pure Newton. Currently, pure Newton fails silently, so how would a user know that there is a problem with their setup (which is potentially a setup that we never tested before) in the first place? Even if it were not silent, what information do we get about the problem by knowing that Newton failed to converge after x-many steps? It potentially does so for many surfaces per helix, so which is even the one we should have hit?

Also, the success of pure Newton is dependent on the starting position, which we estimate with straight line intersection currently. This is potentially inaccurate for low momentum tracks and surfaces that are far away from the track position.

Now, maybe we can be sure that these last two test left that use pure Newton are simple enough that rt_safe will never do anything useful there. But does it harm those tests to the point where we have to keep all of that code duplication around? Also, newton_raphson_safe can always be made to run as pure Newton by failing the root bracketing on purpose, if that is what you prefer?

@beomki-yeo
Copy link
Collaborator

beomki-yeo commented Nov 4, 2024

I thought it complicates the code too much with.
For the first point, having more tolerance values, which may not be required, can be regarded greater? You referred the text book so I can pretend to understand...

Most importantly, for the second point, In the setup of ~ 500 GeV/c and ~ 1T (the harshest setup we can have for now), can there be more types of error other than "there are too many steps and the algorithm does not find intersections yet"? I don't know what your method can do better than the pure Newton method in terms of the error catching.

Copy link

sonarcloud bot commented Nov 15, 2024

@niermann999
Copy link
Contributor Author

I have also added a more generic Newton-Raphson algorithm in this PR, so should I switch the test to that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: low Low priority refactor refactoring the current codes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants