Skip to content

Conversation

@peterkrack
Copy link
Contributor

@peterkrack peterkrack commented Nov 18, 2025

I had some issue when calling pineko theory opcards for two of my grids, when the value y = 6.786550974400577 was passed. The newton method here would not converge in 10 steps.

Now using https://crates.io/crates/lambert_w

@peterkrack peterkrack marked this pull request as draft November 18, 2025 11:49
@felixhekhorn felixhekhorn changed the title replace newton method by analytic solution. Replace Newton method for Lambert function by external package Nov 18, 2025
@cschwan
Copy link
Contributor

cschwan commented Nov 18, 2025

  1. If you found an example where this function doesn't work, it makes for an excellent regression test. Please add it (to the same file) to make sure that we don't repeat the mistake!
  2. In general I recommend to make the minimal changes that fix the problem. In this case especially, because everything is quite sensitive to changes to the x-grid values. Just increase the number of iterations.

@peterkrack
Copy link
Contributor Author

Changing all the check to be compliant with the solution using the lambert W function is pretty hard. Also the functions converning the compatibility with appl grid files seem to get compromised.

Anyway the issue is related to the FMA Fused-multiply-add-operation. Compiling that function in C or Fortran works only on x86. On arm/M1pro the function will fail for certain values and oscillate if using -ffast-math with -ffp-contract=on. On arm/M1 -ffast-math -ffp-contrac=off the function works. The same oscillating behaviour happens in Rust, which does (and should in general imho) do the FMA optimisation. Maybe there is a way to have just this function unoptimised?

main.c
mainf90.txt

@cschwan
Copy link
Contributor

cschwan commented Nov 19, 2025

I believe there's a bug in the Newton's method implementation. The statement delta >= deltap should be: delta.abs() >= delta.abs(). Then the iteration correctly stops, if we also change the limit from 1e-15 to 2e-15.

@cschwan cschwan changed the title Replace Newton method for Lambert function by external package Fix panic in fx2 Nov 20, 2025
@cschwan
Copy link
Contributor

cschwan commented Nov 20, 2025

This is now ready to be merged. Sorry that I reverted your work, @peterkrack, but I think fixing this bug is the easiest solution.

@cschwan cschwan marked this pull request as ready for review November 26, 2025 16:50
@cschwan cschwan merged commit f372b2e into master Nov 26, 2025
10 checks passed
@cschwan cschwan deleted the mucol_dis_with_powheg branch November 26, 2025 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants