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

refactor: improve full_chain_odd.py example #1538

Merged
merged 9 commits into from
Sep 22, 2022

Conversation

andiwand
Copy link
Contributor

  • align odd full chain with itk full chain

see #1513

@andiwand andiwand added this to the next milestone Sep 20, 2022
Copy link
Contributor

@timadye timadye left a comment

Choose a reason for hiding this comment

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

Running with ttbar_pu200 = True, I got:

 PYTHIA Error in BeamRemnants::setKinematics: kinematics construction failed

Do we need to worry about that?

By the way, I noticed that this one has addVertexFitting. Do we want to include that in full_chain_itk.py, or is it still broken for the ITk?

Examples/Scripts/Python/full_chain_odd.py Outdated Show resolved Hide resolved
Examples/Scripts/Python/full_chain_odd.py Outdated Show resolved Hide resolved
Examples/Scripts/Python/full_chain_odd.py Show resolved Hide resolved
Examples/Scripts/Python/full_chain_odd.py Outdated Show resolved Hide resolved
Co-authored-by: Tim Adye <T.J.Adye@rl.ac.uk>
@andiwand
Copy link
Contributor Author

Running with ttbar_pu200 = True, I got:

 PYTHIA Error in BeamRemnants::setKinematics: kinematics construction failed

Do we need to worry about that?

did you see that in the ITk example too? I wonder why this would only show up in the ODD example

@timadye
Copy link
Contributor

timadye commented Sep 20, 2022

did you see that in the ITk example too? I wonder why this would only show up in the ODD example

No, I didn't see it with the ITk example

@andiwand
Copy link
Contributor Author

By the way, I noticed that this one has addVertexFitting. Do we want to include that in full_chain_itk.py, or is it still broken for the ITk?

I think it should work with a similar setup like here after the fixes I added for the vertexing. since we do not have ambiguity resolution in Acts the results are quite funky sometimes but it should produce some OK results

@timadye
Copy link
Contributor

timadye commented Sep 20, 2022

I think it should work with a similar setup like here after the fixes I added for the vertexing. since we do not have ambiguity resolution in Acts the results are quite funky sometimes but it should produce some OK results

Could you add that, so we have the example for the workshop? I can do it if you like, but you know more about this?

@codecov
Copy link

codecov bot commented Sep 20, 2022

Codecov Report

Merging #1538 (1fb5e67) into main (bfc3676) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1538   +/-   ##
=======================================
  Coverage   48.47%   48.47%           
=======================================
  Files         381      381           
  Lines       20699    20699           
  Branches     9503     9503           
=======================================
  Hits        10034    10034           
  Misses       4112     4112           
  Partials     6553     6553           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@andiwand
Copy link
Contributor Author

@timadye I added the vertexing to the ITk example but didn't try it out yet because I don't know how to run it

I also need to look at the pythia error in the ODD example

@timadye
Copy link
Contributor

timadye commented Sep 21, 2022

@timadye I added the vertexing to the ITk example but didn't try it out yet because I don't know how to run it

I can test it!

@andiwand
Copy link
Contributor Author

@timadye I added the vertexing to the ITk example but didn't try it out yet because I don't know how to run it

I can test it!

thank you!

Running with ttbar_pu200 = True, I got:

 PYTHIA Error in BeamRemnants::setKinematics: kinematics construction failed

Do we need to worry about that?

I did't see this error in the first few events. when did it happen for you? @timadye

But I see a bunch of these

13:45:16    TrackFinding   ERROR     Step failed with EigenStepperError:3: Step size adjustment exceeds maximum trials
13:45:16    TrackFinding   ERROR     Propapation failed: EigenStepperError:3 Step size adjustment exceeds maximum trials with the initial parameters 72670 : 
 3.85578
 24.0004
 2.70553
0.682366
-1.54399
-11.8624

which is likely related to #1385 I think

@timadye
Copy link
Contributor

timadye commented Sep 21, 2022

I did't see this error in the first few events. when did it happen for you? @timadye

It comes immediately after the Sequencer initialisation, before the first of the EigenStepperError errors:

13:02:22    Sequencer      INFO      Starting event loop with 1 threads
13:02:22    Sequencer      INFO        0 services
13:02:22    Sequencer      INFO        0 context decorators
13:02:22    Sequencer      INFO        1 readers
13:02:22    Sequencer      INFO        10 algorithms
13:02:22    Sequencer      INFO        12 writers
 PYTHIA Error in BeamRemnants::setKinematics: kinematics construction failed
13:02:27    TrackFinding   ERROR     Step failed with EigenStepperError:3: Step size adjustment exceeds maximum trials

I compiled with the latest key4hep installation (2022-08-22). I just tested again with your latest branch. The only changes are to set ttbar_pu200 = True. (I also reduced the number of events and tried with numThreads=1, but that doesn't affect the error.)

@andiwand
Copy link
Contributor Author

very interesting I cannot reproduce it. it might be due to a different pythia version? I use pythia8307

also just freshly built acts from this branch

I will double check if my acts install was clean

@andiwand
Copy link
Contributor Author

using this setup https://codimd.web.cern.ch/A108z_6tRiWJaIa5yAabdg# I can reproduce the error and there are also a few warnings

 PYTHIA Warning in MultipartonInteractions::init: maximum increased by factor 4.149
...
 PYTHIA Warning in Pythia::check: not quite matched particle energy/momentum/mass  
 PYTHIA Error in BeamRemnants::setKinematics: kinematics construction failed  
 PYTHIA Warning in SimpleSpaceShower::pT2nextQCD: weight above unity  
 PYTHIA Warning in StringFragmentation::fragmentToJunction: bad convergence junction rest frame  
 PYTHIA Warning in SimpleSpaceShower::pT2nextQCD: small daughter PDF

cc @paulgessinger

Copy link
Contributor

@timadye timadye left a comment

Choose a reason for hiding this comment

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

I suggested some changes for full_chain_itk.py, but I guess some of these could also apply to full_chain_odd.py.

Examples/Scripts/Python/full_chain_itk.py Outdated Show resolved Hide resolved
Examples/Scripts/Python/full_chain_itk.py Show resolved Hide resolved
Examples/Scripts/Python/full_chain_itk.py Show resolved Hide resolved
Examples/Scripts/Python/full_chain_itk.py Outdated Show resolved Hide resolved
Examples/Scripts/Python/full_chain_itk.py Outdated Show resolved Hide resolved
Examples/Scripts/Python/full_chain_itk.py Outdated Show resolved Hide resolved
@timadye
Copy link
Contributor

timadye commented Sep 21, 2022

Since you move outputDir to itk_output subdirectory (a nice convenience), can use also specify it for the Sequencer?

s = acts.examples.Sequencer(events=100, numThreads=-1, outputDir=str(outputDir))

That puts the timing.tsv also in itk_output.

@timadye
Copy link
Contributor

timadye commented Sep 21, 2022

@timadye I added the vertexing to the ITk example but didn't try it out yet because I don't know how to run it

with my suggested changes, it seems to run OK for dimuon events or ttbar_pu200. I don't know what sort of performance is to be expected with CKF output. For ttbar_pu200, I'm getting <nRecoVtx>=44 with <nVtxReconstructable>=148.

andiwand and others added 2 commits September 21, 2022 19:21
Co-authored-by: Tim Adye <T.J.Adye@rl.ac.uk>
@andiwand
Copy link
Contributor Author

, outputDir=str(outputDir)

good idea! will add it

with my suggested changes, it seems to run OK for dimuon events or ttbar_pu200. I don't know what sort of performance is to be expected with CKF output. For ttbar_pu200, I'm getting <nRecoVtx>=44 with <nVtxReconstructable>=148.

I am quite surprised that you can reconstruct 44 vertices. @paulgessinger ODD performed worse no? or was that just with AMVF?

Copy link
Contributor

@timadye timadye left a comment

Choose a reason for hiding this comment

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

I don't know if I need to approve this again, but it does no harm I think.

The Pythia error message doesn't seem to stop the job working, so I think we can go ahead with this PR. If required to marge, maybe resolve that thread and when merged, unresolve it to continue the discussion?

@andiwand
Copy link
Contributor Author

as far as I know you need a re-approve after additional changes are made.

The Pythia error message doesn't seem to stop the job working, so I think we can go ahead with this PR. If required to marge, maybe resolve that thread and when merged, unresolve it to continue the discussion?

sounds good will do 👍

@kodiakhq kodiakhq bot merged commit e5d1970 into acts-project:main Sep 22, 2022
@andiwand andiwand deleted the odd_example1 branch September 22, 2022 14:21
@paulgessinger paulgessinger added the backport develop/v19.x Backport this PR to the v19.x series label Sep 22, 2022
@acts-project-service
Copy link
Collaborator

The backport to develop/v19.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-develop/v19.x develop/v19.x
# Navigate to the new working tree
cd .worktrees/backport-develop/v19.x
# Create a new branch
git switch --create backport-1538-to-develop/v19.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 e5d19700e5daa9d60922a2402fe61308eefb7639
# Push it to GitHub
git push --set-upstream origin backport-1538-to-develop/v19.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-develop/v19.x

Then, create a pull request where the base branch is develop/v19.x and the compare/head branch is backport-1538-to-develop/v19.x.

paulgessinger pushed a commit to paulgessinger/acts that referenced this pull request Sep 22, 2022
paulgessinger added a commit that referenced this pull request Sep 22, 2022
…p/v19.x] (#1550)

- align odd full chain with itk full chain

see #1513

Co-authored-by: Andreas Stefl <stefl.andreas@gmail.com>
@paulgessinger paulgessinger modified the milestones: next, v20.2.0 Sep 23, 2022
kodiakhq bot pushed a commit that referenced this pull request Sep 23, 2022
follow-up of #1538

@timadye noticed that we have to use `s.addAlgorithm(acts.examples.TrackSelector(...` in the full chain examples in order to have vertexing. this PR integrates the track selection into the `addVertexFitting` helper
acts-project-service pushed a commit that referenced this pull request Sep 28, 2022
follow-up of #1538

@timadye noticed that we have to use `s.addAlgorithm(acts.examples.TrackSelector(...` in the full chain examples in order to have vertexing. this PR integrates the track selection into the `addVertexFitting` helper

(cherry picked from commit da71b1c)
kodiakhq bot pushed a commit that referenced this pull request Sep 28, 2022
…velop/v19.x] (#1561)

Backport da71b1c from #1545.
---
follow-up of #1538

@timadye noticed that we have to use `s.addAlgorithm(acts.examples.TrackSelector(...` in the full chain examples in order to have vertexing. this PR integrates the track selection into the `addVertexFitting` helper
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport develop/v19.x Backport this PR to the v19.x series
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants