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

feat: Allow volume constrain for propagation #3470

Merged

Conversation

andiwand
Copy link
Contributor

@andiwand andiwand commented Aug 1, 2024

Add optional volume constrains to the propagation which are executed by the VolumeConstraintAborter. This is directly exercised in the track finding and wired to python.

This is generally useful if track finding / fitting should be done in a sub detector region. Same goes for simulation and extrapolation.

@andiwand andiwand added this to the next milestone Aug 1, 2024
@github-actions github-actions bot added Component - Core Affects the Core module Component - Fatras Affects the Fatras module Component - Examples Affects the Examples module Track Finding labels Aug 1, 2024
@andiwand
Copy link
Contributor Author

andiwand commented Aug 1, 2024

I do not observe any performance changes with the ODD

Copy link

github-actions bot commented Aug 1, 2024

📊: Physics performance monitoring for bcb7b53

Full contents

physmon summary

@andiwand andiwand marked this pull request as ready for review August 14, 2024 09:10
@timadye
Copy link
Contributor

timadye commented Aug 22, 2024

Sorry it took so long to get to this PR. I tested it with full_chain_itk.py. Unfortunately it failed right away:

RuntimeError: <class 'acts.ActsPythonBindings._examples.TrackFindingAlgorithm.Config'>: Failed to set pixelVolumeIds={8, 9, 10, 13, 14, 15, 16, 18, 19, 20}

I don't think we necessary need to setup constrainToVolumes for full_chain_itk.py (would be useful, but it's tricky), but it should be fixed to run without.

I noticed that full_chain_odd.py changed the Python type of pixelVolumes and stripVolumes from a set to a list. What's up with that?

@andiwand
Copy link
Contributor Author

Sorry it took so long to get to this PR. I tested it with full_chain_itk.py. Unfortunately it failed right away:

RuntimeError: <class 'acts.ActsPythonBindings._examples.TrackFindingAlgorithm.Config'>: Failed to set pixelVolumeIds={8, 9, 10, 13, 14, 15, 16, 18, 19, 20}

I don't think we necessary need to setup constrainToVolumes for full_chain_itk.py (would be useful, but it's tricky), but it should be fixed to run without.

I will take a look

I noticed that full_chain_odd.py changed the Python type of pixelVolumes and stripVolumes from a set to a list. What's up with that?

I though I make this sound between this PR and the existing volume lists. A small vector is faster to search through and the interface should be more or less stable

@andiwand andiwand marked this pull request as draft August 22, 2024 18:35
@andiwand
Copy link
Contributor Author

@timadye the ITk chain should work again now. The problem was just the change from set to list. Do you want to have another look?

Should we keep the list or revert back to a set? I think it makes sense to have them both use the same container type and a vector seems a bit more preferable for performance here. What do you think?

@timadye
Copy link
Contributor

timadye commented Sep 25, 2024

Should we keep the list or revert back to a set? I think it makes sense to have them both use the same container type and a vector seems a bit more preferable for performance here. What do you think?

Thinking about this, it would break any user scripts that happen to use pixelVolumes or stripVolumes. I expect hardly anyone uses those yet, so it should be OK.

In the end, I think these actually belong in the geometry setup, eg. itk.py so we will have to change again later.

@andiwand andiwand marked this pull request as ready for review September 25, 2024 17:50
@timadye
Copy link
Contributor

timadye commented Sep 25, 2024

This works. With 100 ttbar_pu200 events running full_chain_itk.py, #3470 TrackFindingAlgorithm is 1% slower than its base main (65aff94) and has a 0.7% drop in the fake rate. I don't see an appreciable change in the other tracking performance metrics.

Since we don't specify constrainToVolumes for the ITk, I don't know why the result isn't identical, but it's fine otherwise.

@andiwand
Copy link
Contributor Author

This works. With 100 ttbar_pu200 events running full_chain_itk.py, #3470 TrackFindingAlgorithm is 1% slower than its base main (65aff94) and has a 0.7% drop in the fake rate. I don't see an appreciable change in the other tracking performance metrics.

Since we don't specify constrainToVolumes for the ITk, I don't know why the result isn't identical, but it's fine otherwise.

That is very odd. I just looked at the code and did not see a reason why this might be the case. I will merge main again maybe it got mixed up somehow

@timadye
Copy link
Contributor

timadye commented Oct 2, 2024

Since we don't specify constrainToVolumes for the ITk, I don't know why the result isn't identical, but it's fine otherwise.

That is very odd. I just looked at the code and did not see a reason why this might be the case. I will merge main again maybe it got mixed up somehow

I compared again the last update of #3470 against its last-merged main (0bf2465), using 8 threads as I did before. #3470 is now 1% faster and has a 0.6% increase in the fake rate, both the opposite to what I saw before. Unless this was a mistake on my part, it probably indicates a fluctuation.

I thought the two jobs should be running with the same events, since they have the same random number seed, so should have exactly identical results. But since I am running multi-threaded, there is scope for the events to be generated differently, and hence be subject to random fluctuations.

To test this, I ran again in single-threaded mode (20 events), and got exactly identical results #3470 vs main - even to the distributions being identical. #3470 was 1.6% slower, but I would have to run more longer tests to see if this is actually significant. I don't think it really matters at this level.

Sorry for delaying this PR with the spurious conundrum. It otherwise looks good to me.

timadye
timadye previously approved these changes Oct 2, 2024
Core/include/Acts/Propagator/StandardAborters.hpp Outdated Show resolved Hide resolved
Co-authored-by: Tim Adye <T.J.Adye@rl.ac.uk>
@acts-policybot acts-policybot bot dismissed timadye’s stale review October 2, 2024 14:29

Invalidated by push of 80406de

Co-authored-by: Tim Adye <T.J.Adye@rl.ac.uk>
@paulgessinger
Copy link
Member

@timadye the irreproducibility when running MT should be an Athena effect I guess? Otherwise, this would actually be a bit scary.

@timadye
Copy link
Contributor

timadye commented Oct 2, 2024

@timadye the irreproducibility when running MT should be an Athena effect I guess? Otherwise, this would actually be a bit scary.

I'm running the full chain, so could get events generated in a different order depending on the thread scheduling. Wouldn't that lead to a different random number sequence?

Copy link

sonarqubecloud bot commented Oct 2, 2024

@kodiakhq kodiakhq bot merged commit e7dceab into acts-project:main Oct 2, 2024
42 checks passed
@github-actions github-actions bot removed the automerge label Oct 2, 2024
@andiwand
Copy link
Contributor Author

andiwand commented Oct 2, 2024

I'm running the full chain, so could get events generated in a different order depending on the thread scheduling. Wouldn't that lead to a different random number sequence?

Our RNG should give stable random numbers even tho it is scheduled differently. There is a seed for each event and this will be used consistently in MT or ST

The output order and the root binning might change though

@andiwand andiwand deleted the feat-constrain-propagation-to-volumes branch October 2, 2024 19:10
@acts-project-service acts-project-service added the Fails Athena tests This PR causes a failure in the Athena tests label Oct 2, 2024
@paulgessinger
Copy link
Member

Athena failures seem to be due to cvmfs issues

@paulgessinger paulgessinger modified the milestones: next, v37.0.0 Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Core Affects the Core module Component - Examples Affects the Examples module Component - Fatras Affects the Fatras module Fails Athena tests This PR causes a failure in the Athena tests Track Finding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants