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.

Sign up for GitHub

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

Jump to bottom

fix: Correct the pTperHelixRadius calculation in SeedFinderConfig #2132

Merged
merged 2 commits into from
Jun 13, 2023
Merged

fix: Correct the pTperHelixRadius calculation in SeedFinderConfig #2132

merged 2 commits into from
Jun 13, 2023

Conversation

beomki-yeo
Copy link
Contributor

@beomki-yeo beomki-yeo commented May 17, 2023 •
edited

To synchronize with acts-project/traccc#403

The right equation is rho [mm] / pT [MeV] = 0.0033356 / B[kT] <=> pT[MeV] / rho[mm] = 299.796 * B[kT], so the factor of 300 is wrong

Reference: https://en.wikipedia.org/wiki/Rigidity_(electromagnetism)

@codecov
Copy link

codecov bot commented May 17, 2023 •
edited

Codecov Report

Merging #2132 (36c847b) into main (43d5adc) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2132   +/-   ##
=======================================
  Coverage   49.41%   49.41%           
=======================================
  Files         441      441           
  Lines       25176    25176           
  Branches    11617    11617           
=======================================
  Hits        12441    12441           
  Misses       4481     4481           
  Partials     8254     8254

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

@LuisFelipeCoelho LuisFelipeCoelho self-requested a review May 17, 2023 12:57
LuisFelipeCoelho
LuisFelipeCoelho suggested changes May 17, 2023
View reviewed changes
Copy link
Member

@LuisFelipeCoelho LuisFelipeCoelho left a comment •
edited

Choose a reason for hiding this comment

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

Athena uses the 300 factor, so to keep the same performance you also need to change the magnetic field in the ITk configuration. Or we can discuss changing that value in Athena as well

@LuisFelipeCoelho
Copy link
Member

FYI @noemina

@LuisFelipeCoelho
Copy link
Member

Also @beomki-yeo could you change the name of this PR to make it more informative?

@beomki-yeo beomki-yeo changed the title fix: Fix a bug in seeding fix: Correct the pTperHelixRadius calculation in SeedFinderConfig May 17, 2023
@beomki-yeo
Copy link
Contributor Author

beomki-yeo commented May 17, 2023 •
edited

In case people want to see the physmon result. The change is very small

https://drive.google.com/drive/folders/1ngk3lpti-lYo1PeJFRkaGyTPJqVenLVB?usp=sharing

@beomki-yeo
Copy link
Contributor Author

As @andiwand already implied in the comment, we need to stop using the internal unit system at some point.

@paulgessinger
Copy link
Member

What's the status of this? Can we move ahead?

@paulgessinger paulgessinger added this to the next milestone May 24, 2023
@LuisFelipeCoelho
Copy link
Member

I am in favor of changing this, but I wanted to avoid diverging from Athena:
So I suggested changing this in Athena as well, but after discussion with Noemi, I think it will be complicated to change this now that the reconstruction is almost ready for the note, and this will require studies to support the change.

Since it is a small change, I think we can change it in here and ignore it in Athena.

@beomki-yeo
Copy link
Contributor Author

I would like to go ahead but I need to replace the physics files. I don't know why the physmon results do not show up 🤔

@paulgessinger
Copy link
Member

Not sure why the comment is missing. This is handled by a secondary github actions that's triggered separately, maybe there was a problem?

@beomki-yeo
Copy link
Contributor Author

Is it triggered only when linux_physmon fails? Then it makes sense

@paulgessinger
Copy link
Member

Should always be triggered in principle.

@beomki-yeo
Copy link
Contributor Author

Well I can still generate the files by myself hopefully

@andiwand
Copy link
Contributor

andiwand commented May 26, 2023 •
edited

Well I can still generate the files by myself hopefully

You can also download them from the job artifact even if the physmon post is not showing up

Looks like examples tests are still failing https://github.com/acts-project/acts/actions/runs/5003699620/jobs/8965554424?pr=2132

At the bottome of this page you can find the artifacts https://github.com/acts-project/acts/actions/runs/5003699620?pr=2132

@github-actions
Copy link

github-actions bot commented May 31, 2023 •
edited

📊 Physics performance monitoring for 36c847b

Summary
Full report
Seeding: seeded, truth estimated, orthogonal
CKF: seeded, truth smeared, truth estimated, orthogonal
IVF: seeded, truth smeared, truth estimated, orthogonal
AMVF: seeded, truth smeared, truth estimated, orthogonal
Ambiguity resolution: seeded, orthogonal
Truth tracking
Truth tracking (GSF)

Vertexing

Vertexing vs. mu
IVF seeded

IVF truth_smeared

IVF truth_estimated

IVF orthogonal

AMVF seeded

AMVF truth_smeared

AMVF truth_estimated

AMVF orthogonal

Seeding

Seeding seeded

Seeding truth_estimated

Seeding orthogonal

CKF

CKF seeded

CKF truth_smeared

CKF truth_estimated

CKF orthogonal

Ambiguity resolution

seeded

Truth tracking (Kalman Filter)

Truth tracking

Truth tracking (GSF)

Truth tracking

@andiwand andiwand added the Component - Core Affects the Core module label Jun 1, 2023
@andiwand
Copy link
Contributor

there is another conflict @beomki-yeo . I think we should get this in tomorrow otherwise it will just keep conflicting...

if there are no objections I will approve and merge as soon as this is green again

cc @LuisFelipeCoelho

@github-actions github-actions bot added the Component - Examples Affects the Examples module label Jun 12, 2023
@kodiakhq kodiakhq bot merged commit 765cad0 into acts-project:main Jun 13, 2023
50 checks passed
@beomki-yeo beomki-yeo mentioned this pull request Jun 15, 2023
Merged
@paulgessinger paulgessinger modified the milestones: next, v27.0.0 Jun 20, 2023
@LuisFelipeCoelho LuisFelipeCoelho mentioned this pull request Jun 26, 2023
Merged
kodiakhq bot pushed a commit that referenced this pull request Jun 26, 2023
@LuisFelipeCoelho
refactor: Using UnitConstants in calculation of minHelixRadius in Spa…
b483c1a 
…cePointGrid (#2243)

Same as #2132
noemina pushed a commit to noemina/acts that referenced this pull request Jul 5, 2023
@LuisFelipeCoelho @noemina
refactor: Using UnitConstants in calculation of minHelixRadius in Spa…
6a42eb4 
…cePointGrid (acts-project#2243)

Same as acts-project#2132
@andiwand andiwand mentioned this pull request Jul 11, 2023
Merged
@andiwand andiwand mentioned this pull request Oct 25, 2023
Merged
andiwand added a commit that referenced this pull request Oct 26, 2023
@andiwand @kodiakhq
refactor: Replace magic seeding field in Examples (#2577)
0027d4c 
I believe this is not accurate anymore since we switched out the magic
number in the seeding

#2132

---------

Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
@StewMH StewMH mentioned this pull request Apr 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stephenswat stephenswat stephenswat left review comments

@andiwand andiwand andiwand approved these changes

@LuisFelipeCoelho LuisFelipeCoelho LuisFelipeCoelho approved these changes

Assignees
No one assigned
Labels
Component - Core Affects the Core module Component - Examples Affects the Examples module
Projects
None yet
Milestone
v27.0.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@beomki-yeo @LuisFelipeCoelho @paulgessinger @andiwand @stephenswat