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: Enable Fatras interactions by default / make configurable in addFatras(...) #1631

Merged

Conversation

benjaminhuth
Copy link
Member

It turned out that all interactions in Fatras where turned off by default... This is not optimal I think. This PR enables these by default both on the C++ site and on the python side, to be explicit.

As a consequence, all root hashes relying on FATRAs should change...

@benjaminhuth benjaminhuth added Improvement Changes to an existing feature Component - Examples Affects the Examples module Impact - Major Significant bug and/or affects a lot of modules labels Oct 27, 2022
@benjaminhuth benjaminhuth added this to the next milestone Oct 27, 2022
@codecov
Copy link

codecov bot commented Oct 27, 2022

Codecov Report

Merging #1631 (ee53840) into main (12c2774) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1631   +/-   ##
=======================================
  Coverage   48.54%   48.54%           
=======================================
  Files         384      384           
  Lines       21042    21042           
  Branches     9693     9693           
=======================================
  Hits        10214    10214           
  Misses       4106     4106           
  Partials     6722     6722           

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

@github-actions
Copy link

github-actions bot commented Oct 27, 2022

📊 Physics performance monitoring for ee53840

Full report
CKF: seeded, truth smeared, truth estimated
IVF: seeded, truth smeared, truth estimated
Ambiguity resolution
Truth tracking

Vertexing

IVF seeded

IVF truth smeared

IVF truth estimated

CKF

seeded

truth smeared

truth estimated

Ambiguity resolution

seeded

Truth tracking

Truth tracking

@benjaminhuth
Copy link
Member Author

If I see it correctely in the physics performance monitor, this has significant impact with respect to some plots, e.g. qop for truth-tracking, this looks not really good...
I'm not sure how to proceed from here actually. Is this expected? Is this a problem of Fatras or of the Algorithms? What do you think, @asalzburger @paulgessinger ?

@andiwand andiwand added 🚧 WIP Work-in-progress and removed automerge labels Oct 27, 2022
@andiwand
Copy link
Contributor

the downstream performance does not look so bad but truth tracking q/p looks really bad

image

it was biased in the first place but with those changes even more.

@benjaminhuth do you think it makes sense to try a subset of the interactions?

@benjaminhuth
Copy link
Member Author

Could make sense... We use only muons here, right? so effects like bremsstrahlung and pair production should not be important anyways (I think?). However, it might be worth to see if maybe ionization loss or multiple scattering has a bigger impact.

However, I had a quick chat with @paulgessinger and I seems there are a lot of possible reasons for that, including also the material description etc...

Running with Geant could also be good to check. But acutally I do not have the capacity right now to investigate this in-depth...

@andiwand
Copy link
Contributor

Running with Geant could also be good to check. But acutally I do not have the capacity right now to investigate this in-depth...

I agree running Geant4 would be interesting. we can let this rest for now until someone runs into this as well or you want to follow up when you have more time

@paulgessinger
Copy link
Member

Thoughts @asalzburger?

@benjaminhuth benjaminhuth changed the title fix: Enable Fatras interactions by default fix: Enable Fatras interactions by default / make configurable in addFatras(...) Oct 28, 2022
@benjaminhuth
Copy link
Member Author

Hmm how about we go like this: We introduce a flag in the addFatras(...) method to let this be enabled easily (e.g. for running the GSF or other testing), but is off by default to let the physmon pass.
For these physics issues we open an issue, and discuss it there.
What do you think @andiwand @paulgessinger @asalzburger ?

@andiwand
Copy link
Contributor

I agree 👍 ping me if you need a review

@benjaminhuth benjaminhuth added Impact - Minor Nuissance bug and/or affects only a single module and removed Impact - Major Significant bug and/or affects a lot of modules labels Nov 1, 2022
@benjaminhuth benjaminhuth changed the title fix: Enable Fatras interactions by default / make configurable in addFatras(...) refactor: Enable Fatras interactions by default / make configurable in addFatras(...) Nov 1, 2022
@benjaminhuth
Copy link
Member Author

@andiwand I think this is ready to review (mainly the title has changed to be more appropriate^^)

@andiwand andiwand removed the 🚧 WIP Work-in-progress label Nov 1, 2022
@kodiakhq kodiakhq bot merged commit 79cabe0 into acts-project:main Nov 4, 2022
@github-actions github-actions bot removed the automerge label Nov 4, 2022
@benjaminhuth benjaminhuth deleted the fix/enable-interactions-fatras branch November 9, 2022 13:27
@paulgessinger paulgessinger modified the milestones: next, v21.1.0 Nov 11, 2022
@beomki-yeo
Copy link
Contributor

@paulgessinger @benjaminhuth In physmon truth tracking kalman, I got this:

  1. Eta fixed to 0, all physics disabled in simulation
    image

  2. Eta fixed to 0, all physics enabled in simulation
    image

Both don't look right. Let me think about it more...

@beomki-yeo
Copy link
Contributor

nce does not look so bad but truth tracking q/p looks really bad

@andiwand Actually the monitored graph looks better than reference one (the sigma is closer to 1)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Examples Affects the Examples module Impact - Minor Nuissance bug and/or affects only a single module Improvement Changes to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants