8000 Adapt test tolerances to true Float64 precision by ddahlbom · Pull Request #360 · SunnySuite/Sunny.jl · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Adapt test tolerances to true Float64 precision #360

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
Merged
merged 2 commits into from
Apr 18, 2025
Merged

Adapt test tolerances to true Float64 precision #360

merged 2 commits into from
Apr 18, 2025

Conversation

ddahlbom
Copy link
Member
@ddahlbom ddahlbom commented Mar 25, 2025 •
edited by kbarros
Loading

Loosens the bounds on the "Kitchen Sink" LSWT dispersion test. Removes g_tol from call to minimize_energy! for dimers and also loosens the test bounds somewhat.

Curiously, removing the g_tol and using defaults did not result in a sufficient optimum, even though explicitly setting x_tol=1e-14 did produce sufficiently accurate results. This seems to indicate that the default settings (with x_tol=0 and the secret NaN configuration) worked less well than having a finite x_tol. This is a bit odd.

@kbarros
Copy link
Member
kbarros commented Mar 26, 2025 •
edited
Loading

These changes look good.

Note that minimize_energy! uses a default value of x_tol=1e-12, as shown here:

Sunny.jl/src/Optimization.jl

Line 133 in 01cacf5

options = Optim.Options(; iterations=subiters, x_tol=1e-12, g_tol=0, f_reltol=NaN, f_abstol=NaN, kwargs...)

Therefore it makes sense that you are getting about 12 digits of accuracy in the minimized spin magnitude.

I can't remember if there was a good reason for Sunny to select x_tol=1e-12 specifically. (Perhaps if x_tol becomes too small, then minimize_energy! starts to hit convergence errors.) It's nice to know that you were able to manually make x_tol smaller to achieve more accuracy for this problem.

kbarros
kbarros reviewed Mar 26, 2025
View reviewed changes
test/test_lswt.jl Outdated
@@ -43,7 +43,7 @@
@assert energy_per_site(sys) ≈ -328.38255

# Verify that this is a local minimum of energy
@test norm(Sunny.proj.(Sunny.energy_grad_coherents(sys), sys.coherents)) < 1e-8
@test norm(Sunny.proj.(Sunny.energy_grad_coherents(sys), sys.coherents)) < 2e-8
Copy link
Member

Choose a reason for hiding this comment

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

On my Mac, I verified we can get many more digits of accuracy here by selecting, say, x_tol=1e-16 inside minimize_energy!.

test/test_lswt.jl Outdated
@@ -54,8 +54,8 @@
# println(round.(vec(res.data); digits=12))
disps_golden = [1394.440092579932, 1393.728009951748, 1393.008551251227, 1392.919524974106, 1279.239919068656, 1279.094568472208, 1278.224518515369, 1277.691761482934, 1194.366336255048, 1193.750083625344, 1191.583519659752, 1189.794451340786, 1131.422439587814, 1131.202770074483, 1065.242927850645, 1065.095892446623, 1026.649340922942, 1024.028348558074, 1022.830406299683, 1020.767349639389, 945.202397530637, 944.795817851532, 835.545028394177, 832.001588695239, 827.939501409159, 827.307586947865, 821.216582166477, 820.430993567146, 820.294548776124, 818.594570998049, 810.207001090183, 808.553158273676, 766.524411071072, 766.516102750272, 766.51382585253, 766.508655558251, 758.5798541677, 754.683765885704, 750.572578891224, 750.47100625256, 665.954573008178, 662.421047653195, 651.465562549975, 651.417940124351, 581.258189152584, 568.105209800095, 559.053702296455, 558.493005822971, 552.043762736843, 550.131096070956, 539.733572947825, 530.698033192909, 499.661483510074, 494.928560823138, 435.233706061892, 427.702277064325, 408.128705853663, 399.856401749648, 370.069343063308, 369.845327686246, 365.049514240266, 363.639416669427, 354.648012591404, 346.609483927002, 341.989165167266, 339.373361067994, 318.363717384335, 276.219249203178, 263.1610538318, 257.409506246766, 230.539454193854, 229.778324172693, 203.971681278992, 197.504237153931, 193.879371534726, 189.866421875022, 189.815806967694, 167.94413443161, 154.923566498384, 146.21953884777]
data_golden = [0.00038665026, 0.0, 0.007231537047, 0.0, 0.008665001888, 0.0, 0.015340530772, 0.0, 0.0, 0.054200267869, 0.073309637341, 0.0, 0.005276014091, 0.0, 0.0, 0.026708463804, 0.0, 0.062332773978, 0.112027719874, 0.0, 0.031129659091, 0.0, 0.115569104228, 0.0, 0.0, 0.038994974901, 0.0, 0.031152494381, 0.029113142876, 0.0, 0.0, 0.00467095768, 0.000762927464, 0.000940556177, 0.0, 0.0, 0.00877327198, 0.0, 0.033263081608, 0.0, 0.066082588247, 0.0, 0.0, 0.059956690592, 0.334527718881, 0.007608761425, 0.0, 0.0, 0.0, 0.068446181755, 0.029366565904, 0.0, 0.0, 0.605679689903, 0.378667970539, 0.0, 0.858558512424, 0.0, 0.0, 0.290056738762, 0.349117318827, 0.0, 0.0, 0.955658357376, 0.0, 1.606184739756, 0.0, 0.201711065754, 0.181786511323, 0.0, 0.0, 0.313711125445, 0.769401980923, 0.0, 0.010377681107, 0.0, 0.151200728665, 0.327686235743, 0.0, 0.0]
@test isapprox(res.disp, disps_golden; atol=1e-9)
@test isapprox(res.data, data_golden; atol=1e-9)
@test isapprox(res.disp, disps_golden; atol=2e-8)
Copy link
Member

Choose a reason for hiding this comment

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

This is a curious degradation -- I wonder if it would be resolved by tightening x_tol in the minimize_energy! call?

Copy link
Member

Choose a reason for hiding this comment

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

On second thought, maybe the "golden" numbers are themselves not converged to high precision? (That would be the case if they predate our upgrades to Optim convergence parameters.)

I suggest we regenerate them at some higher precision level, like x_tol=1e-14, and then see if we can get better agreement between Arm and Intel.

Copy link
Member Author
@ddahlbom ddahlbom Mar 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

After explicitly setting x_tol=1e-14 and refreshing the reference data, the tests pass on my Intel machines (Linux and Windows) as well as on my M2 Mac with the original tolerance.

Copy link
Member

Choose a reason for hiding this comment

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

While keeping these updates to the reference data, can you please check if the tests pass when x_tol is reduced to its default value (which is 1e-12)?

Copy link
Member Author
@ddahlbom ddahlbom Mar 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The tests do not pass if the tolerance is relaxed back to 1e-12 (on my Linux workstation).

Copy link
Member Author

Choose a reason for hiding this comment

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

They do however pass (with x_tol=1e-12) on the Mac.

@ddahlbom
Copy link
Member Author

Thanks for looking this over. I can try refreshing the golden references with newly optimized data and compare across platforms later this week.

@kbarros
Copy link
Member
kbarros commented Apr 1, 2025

We might want to hold off a bit before modifying the thresholds. I'm discussing with pkofod about the Optim.jl tolerance parameters. Maybe if we could replace x_tol with, say, x_reltol then this could achieve higher accuracy on all platforms.

@quantumsteve
Copy link

We might want to hold off a bit before modifying the thresholds. I'm discussing with pkofod about the Optim.jl tolerance parameters. Maybe if we could replace x_tol with, say, x_reltol then this could achieve higher accuracy on all platforms.

I'm seeing a warning about x_tol being deprecated. Do you already have a plan to update this?

 Warning: x_tol is deprecated. Use x_abstol or x_reltol instead. The provided value (1.0e-12) will be used as x_abstol.           
 @ Optim /opt/julia/packages/Optim/8dE7C/src/types.jl:110

@ddahlbom
Copy link
Member Author
ddahlbom commented Apr 14, 2025 •
edited
Loading

Thanks for pointing that out @quantumsteve. I haven't seen those warnings myself. I'll make sure my Optim is updated. It makes sense to change instances of x_tol to x_abstol if that's the case.

@kbarros Is there any new information from pkofod? I just started a new branch for Paddison's feature request, working off of the current main, and was reminded that I shouldn't expect the tests to pass on my Intel machine.

@kbarros
Copy link
Member
kbarros commented Apr 14, 2025 •
edited
Loading

I don't think x_reltol or similar will be available (in LineSearches) in the near future, and this PR is probably the best strategy for now. I'll try to get to it within a couple days.

@kbar B0D9 ros kbarros changed the title Fix golden tests on Intel machines Adapt test tolerances to true Float64 precision Apr 18, 2025
@kbarros
Copy link
Member
kbarros commented Apr 18, 2025

This PR is also needed to make tests pass on Julia 1.12 on Mac.

@kbarros kbarros force-pushed the check-formfactors-consistent branch from cdd7337 to 00e52a6 Compare April 18, 2025 20:13
@kbarros kbarros merged commit c7aee62 into main Apr 18, 2025
4 checks passed
@kbarros kbarros deleted the check-formfactors-consistent branch April 18, 2025 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kbarros kbarros kbarros left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ddahlbom @kbarros @quantumsteve
0