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: Tweak AMVF config with time in Examples #2985

Merged
merged 14 commits into from
Mar 13, 2024

Conversation

andiwand
Copy link
Contributor

@andiwand andiwand commented Feb 23, 2024

I observed higher vertex efficiency with time setting maxMergeVertexSignificance to default. tracksMaxSignificance and doFullSplitting were also reset in order to use the same config with and without time.

@andiwand andiwand added this to the next milestone Feb 23, 2024
@andiwand
Copy link
Contributor Author

Let's see what the physmon performance says

@github-actions github-actions bot added Component - Examples Affects the Examples module Vertexing labels Feb 23, 2024
Copy link

codecov bot commented Feb 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 48.84%. Comparing base (3a4c1d4) to head (1c508ec).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2985   +/-   ##
=======================================
  Coverage   48.84%   48.84%           
=======================================
  Files         492      492           
  Lines       28861    28861           
  Branches    13685    13685           
=======================================
  Hits        14096    14096           
  Misses       4955     4955           
  Partials     9810     9810           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

paulgessinger
paulgessinger previously approved these changes Feb 23, 2024
Copy link
Member

@paulgessinger paulgessinger left a comment

Choose a reason for hiding this comment

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

Physmon seems compatible with efficiency increase.

@andiwand
Copy link
Contributor Author

After discussion with @pbutti we agreed on splitting the change to see what option drives what performance.

One problem with our current monitoring is that we do not know if the additional vertices are duplicates or fakes. We will have to dig a bit deeper to check that.

@andiwand andiwand changed the title fix: Tweak AMVF config with time in Examples fix: Tweak AMVF config maxMergeVertexSignificance with time in Examples Feb 23, 2024
felix-russo
felix-russo previously approved these changes Feb 24, 2024
Copy link
Contributor

@felix-russo felix-russo left a comment

Choose a reason for hiding this comment

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

lgtm

For lower pile up (i.e., mu=50 like in our physmon), I used to see a better performance when this line was there

For higher pile-up (i.e., mu = 200 like in the setup for the paper) the performance was better without it

If now the performance is better for low pile-ups as well, we should definitely remove it

If we encounter bad performance in a new setup, I would always consider tweaking this parameter too see what happens. Adding a python binding might be good in the future.

@andiwand andiwand force-pushed the tweak-amvf-options-with-time branch from 4dc7c56 to 65aa7f0 Compare March 4, 2024 19:34
@andiwand andiwand changed the title fix: Tweak AMVF config maxMergeVertexSignificance with time in Examples fix: Tweak AMVF config with time in Examples Mar 4, 2024
@andiwand andiwand added the 🚧 WIP Work-in-progress label Mar 5, 2024
@andiwand
Copy link
Contributor Author

andiwand commented Mar 6, 2024

physmon looks good now

@github-actions github-actions bot added the Component - Core Affects the Core module label Mar 12, 2024
@andiwand andiwand removed the 🚧 WIP Work-in-progress label Mar 13, 2024
@github-actions github-actions bot added Infrastructure Changes to build tools, continous integration, ... Changes Performance labels Mar 13, 2024
@kodiakhq kodiakhq bot merged commit dac4a44 into acts-project:main Mar 13, 2024
54 checks passed
@andiwand andiwand deleted the tweak-amvf-options-with-time branch March 13, 2024 19:23
dimitra97 pushed a commit to dimitra97/acts that referenced this pull request Mar 19, 2024
I observed higher vertex efficiency with time setting `maxMergeVertexSignificance` to default. `tracksMaxSignificance` and `doFullSplitting` were also reset in order to use the same config with and without time.
@paulgessinger paulgessinger modified the milestones: next, v33.1.0 Mar 26, 2024
EleniXoch pushed a commit to EleniXoch/acts that referenced this pull request May 6, 2024
I observed higher vertex efficiency with time setting `maxMergeVertexSignificance` to default. `tracksMaxSignificance` and `doFullSplitting` were also reset in order to use the same config with and without time.
asalzburger pushed a commit to asalzburger/acts that referenced this pull request May 21, 2024
I observed higher vertex efficiency with time setting `maxMergeVertexSignificance` to default. `tracksMaxSignificance` and `doFullSplitting` were also reset in order to use the same config with and without time.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changes Performance Component - Core Affects the Core module Component - Examples Affects the Examples module Infrastructure Changes to build tools, continous integration, ... Vertexing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants