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: Replace magic seeding field in Examples #2577

Merged
merged 6 commits into from
Oct 26, 2023

Conversation

andiwand
Copy link
Contributor

@andiwand andiwand commented Oct 24, 2023

I believe this is not accurate anymore since we switched out the magic number in the seeding

#2132

@andiwand andiwand added this to the next milestone Oct 24, 2023
@github-actions github-actions bot added Component - Core Affects the Core module Infrastructure Changes to build tools, continous integration, ... Component - Examples Affects the Examples module Component - Plugins Affects one or more Plugins Seeding labels Oct 24, 2023
@codecov
Copy link

codecov bot commented Oct 24, 2023

Codecov Report

Merging #2577 (9962360) into main (df34a3a) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2577   +/-   ##
=======================================
  Coverage   49.63%   49.63%           
=======================================
  Files         471      471           
  Lines       26687    26687           
  Branches    12277    12277           
=======================================
  Hits        13245    13245           
  Misses       4746     4746           
  Partials     8696     8696           

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

@AJPfleger
Copy link
Contributor

I looked how we introduced this number. First Appearance in Acts seems to be here (pre-PR time):
fa68459

@paulgessinger suggests that the value could be the real ATLAS-field (maybe taken from Athena).

@andiwand
Copy link
Contributor Author

andiwand commented Oct 25, 2023

I looked how we introduced this number. First Appearance in Acts seems to be here (pre-PR time): fa68459

@paulgessinger suggests that the value could be the real ATLAS-field (maybe taken from Athena).

if I remember correctly @beomki-yeo found a magic number (300) in the seeding implementation which was an approximation for c and exchanged it with the physical value. but we never changed the field with compensated the difference

see #2132

@AJPfleger
Copy link
Contributor

AJPfleger commented Oct 25, 2023

I am still not sure, how the numbers relate. According to #2132 this relation should make sense c * B = const:

299.792458 * 2 = 300 * 1.998616... ~ 300 * 1.99724 (quite off)

Using c^2 * B = const:
299.792458^2 * 2 = 300^2 * 1.9972337... ~ 300^2 * 1.99724 (yes the rounding is wrong)
However, I don't understand the physical basis for this.

@andiwand
Copy link
Contributor Author

right that does not add up. I could imagine that this value is the product of some kind of optimization

Copy link
Contributor

@AJPfleger AJPfleger left a comment

Choose a reason for hiding this comment

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

Let's see, if this changes physics^^

@andiwand
Copy link
Contributor Author

it is only affecting the examples. I updated the ttbar vertexing histograms. they are quite noisy as they are low stat. otherwise it should be uncontroversial and we have one magic number less

@andiwand andiwand changed the title refactor: Replace magic seeding field refactor: Replace magic seeding field in Examples Oct 26, 2023
@andiwand andiwand merged commit 0027d4c into acts-project:main Oct 26, 2023
54 checks passed
@andiwand andiwand deleted the replace-magic-seeding-field branch October 26, 2023 20:17
@acts-project-service acts-project-service added the Fails Athena tests This PR causes a failure in the Athena tests label Oct 26, 2023
@paulgessinger paulgessinger removed this from the next milestone Nov 6, 2023
@paulgessinger paulgessinger added this to the v31.0.0 milestone Nov 6, 2023
@paulgessinger paulgessinger removed the Fails Athena tests This PR causes a failure in the Athena tests label Nov 16, 2023
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 Component - Plugins Affects one or more Plugins Infrastructure Changes to build tools, continous integration, ... Seeding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants