Skip to content

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

Open
gaoflow wants to merge 1 commit into
CalebBell:masterfrom
gaoflow:fix-papaevangelo-log10-numerator
Open

Fix Papaevangelo_2010 friction factor: log10 not natural log in numerator#97
gaoflow wants to merge 1 commit into
CalebBell:masterfrom
gaoflow:fix-papaevangelo-log10-numerator

Conversation

@gaoflow

@gaoflow gaoflow commented Jul 21, 2026

Copy link
Copy Markdown

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.

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