Skip to content

Fix Papaevangelo_2010 friction factor: log10 not natural log in numerator - #97

Merged
CalebBell merged 2 commits into
CalebBell:masterfrom
binggao1230:fix-papaevangelo-log10-numerator
Jul 26, 2026
Merged

CalebBell merged 2 commits into
CalebBell:masterfrom
binggao1230:fix-papaevangelo-log10-numerator

Conversation

@binggao1230

Copy link
Copy Markdown
Contributor

Papaevangelo_2010 computes the numerator term (7 - log Re)^4 with math.log (natural log), but the denominator and the original correlation use log10. The result is wrong across the method's registered range (Re 1e4-1e7, eD 1e-5-1e-3): up to ~260% error versus Colebrook, and negative friction factors above Re ~ 3e6.

>>> from fluids.friction import Papaevangelo_2010
>>> Papaevangelo_2010(1e7, 1e-4)
-0.019956280684600055   # a negative friction factor, inside the valid range

Switching the numerator to log10 drops the max error over the range to ~0.6%, matching the correlation's stated accuracy. I also corrected the docstring formula (it showed \ln) and the doctest / test_friction reference values, which were both baked from the buggy output. Added a range sweep asserting the correlation stays positive and within 1% of Colebrook across its domain.

The numerator term (7 - log Re)^4 used math.log (natural log) while the
denominator and the source correlation use log10. Over the method's
registered range (Re 1e4-1e7, eD 1e-5-1e-3) this gave errors up to ~260%
versus Colebrook and negative friction factors above Re~3e6.

Switch the numerator to log10 (max error drops to ~0.6%) and fix the
docstring formula. The doctest and unit-test reference values were baked
from the buggy output; recompute them. Add a range sweep asserting the
correlation stays positive and within 1% of Colebrook across its domain.
@CalebBell

Copy link
Copy Markdown
Owner

Thank you! Nice catch. The CI failure show that the Colebrook(Re, eD, tol=0) call uses mpmath, but this test isn't masked as requiring mpmath so it tries to run on platforms without mpmath and fails. Colebrook is very high precision by itself. Can you remove the tol=0 argument? Otherwise this looks good to merge.

@binggao1230

binggao1230 commented Jul 26, 2026 •

Copy link
Copy Markdown
Contributor Author

Removed — 1d3f3c1. The default-tolerance Colebrook is plenty as a reference here; worst-case deviation across the grid is 0.57% (at Re=1e5, eD=1e-3), inside the 1% assertion, and the test no longer pulls in mpmath.

@CalebBell
CalebBell merged commit 5fa218f into CalebBell:master Jul 26, 2026
61 of 62 checks passed
@CalebBell

Copy link
Copy Markdown
Owner

Thank you for this fix!

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.

2 participants