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

Update and centralize the C constants #691

Merged
merged 10 commits into from
Nov 16, 2023
Merged

Update and centralize the C constants #691

merged 10 commits into from
Nov 16, 2023

Conversation

JeanLucPons
Copy link
Collaborator

Hello,

I centralized constants used in at in an include file atconstants.h

I'm facing an issue concerning the CGAMMA constant defined as below:
Physics of Electron Storage Ring, M. Sands (4.2)
image

In the atconstants header file, the result of the formula gives: 8.846273834e-05 and it AT the old define was 8.846056192e-05.
The new cgamma constant results in at_physics test failure:

test_physics.py ...........FFF.....F..............FF

The best match i can get (8.846092697e-05) is by using 2.8179e-15m for the classical e- radius and 511keV for the rest energy.

Should we modify the test_physics.py or rather adjusting the constants above ?

Note: The very small correction of symplectic integrator constants does not affect the result of the tests.
Thanks

@swhite2401
Copy link
Contributor

Hello @JeanLucPons , this is a nice re-organization.

Concerning the constant, if it is wrong (@lfarv @lnadolski , any idea why??) it has to be corrected and the unit tests modified accordingly.

@lfarv
Copy link
Contributor

lfarv commented Nov 8, 2023

Should we modify the test_physics.py or rather adjusting the constants above ?

I am rather skeptical about "adjusting" the physical constants…

PyAT (in python) uses the correct value of Cγ, computed in at/pyat/at/constants.py. It uses the last definition of the physical constants (2018) from SciPy, based on CODATA 2018 values. And the result is 8.846273822420375e-32, you are almost right, @JeanLucPons… The value in C must date from the previous release of the physical constants (2014) or even before…

So the correct value should of course be set, and the tests corrected accordingly. But the fact that the AT results will change must be clearly documented, it may raise complains…

@lfarv
Copy link
Contributor

lfarv commented Nov 8, 2023

The physical constants were updated in Matlab in #437, and I remember it raised many questions. But they probably were never updated in C

@JeanLucPons
Copy link
Collaborator Author

Thanks Laurent, I also prefer to update the physical constants to the good values and update the unit tests.
I will copy the constants from the Pr you mentioned.

This was referenced Nov 9, 2023
@lfarv lfarv changed the title Centralize constants Update and centralize the C constants Nov 10, 2023
@lfarv lfarv added Matlab For Matlab/Octave AT code Python For python AT code labels Nov 10, 2023
@lfarv
Copy link
Contributor

lfarv commented Nov 16, 2023

The tests are modified using results from Matlab. No tolerance had to be modified (good point!).

Please approve, to unlock the pending Matlab and python releases.

@lfarv lfarv merged commit 0301367 into master Nov 16, 2023
31 checks passed
@lfarv lfarv deleted the centralize_constants branch November 16, 2023 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Matlab For Matlab/Octave AT code Python For python AT code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants