Fix Papaevangelo_2010 friction factor: log10 not natural log in numerator#97
Open
gaoflow wants to merge 1 commit into
Open
Fix Papaevangelo_2010 friction factor: log10 not natural log in numerator#97gaoflow wants to merge 1 commit into
gaoflow wants to merge 1 commit into
Conversation
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.
Owner
|
Thank you! Nice catch. The CI failure show that the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Papaevangelo_2010computes the numerator term(7 - log Re)^4withmath.log(natural log), but the denominator and the original correlation uselog10. 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 aboveRe ~ 3e6.Switching the numerator to
log10drops 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_frictionreference values, which were both baked from the buggy output. Added a range sweep asserting the correlation stays positive and within 1% ofColebrookacross its domain.