Refactor Fanno flow calculation and remove short-tube approximation - #8
Merged
Merged
Conversation
…ion, bad mdot formula, and continuity break Three bugs in the original _fanno_state implementation masked the K-loss effect: 1. outlet_pressure() divided exit pressure by (1+K_out)(1+K_in) a second time even though K losses were already encoded in f4ld. This double-counting caused the subsonic bisection to return 0 for pressure ratios 0.35–0.59, so _fanno_state was silent over most of the subsonic regime. 2. The final mdot formula used P_up (stagnation) with T1 (static temperature), overestimating mass flow by the factor (P0/P1) ≈ 1.14 at M=0.44. Using the correct static inlet pressure P1 gives mdot_fanno/mdot_lossy ≈ 0.85 at P_dn=0 (matching the 0.000313 vs 0.000368 kg/s values in the issue). 3. An early fallback at P_dn/P_up >= 0.6 returned the lossy-nozzle result, creating a sharp discontinuity (≈8.8% step) at r=0.60 once K losses were correctly included in f4ld. Removing this fallback makes _fanno_state compute the Fanno solution uniformly across all pressure ratios, restoring monotonicity. Also removed the 0.99×md_l floor in mdot_fanno_tube that prevented Fanno from dominating, and restored correct test thresholds (ratio < 0.92, strict prev-1e-12 monotonicity). Updated test_fanno_low_mach_matches_lossy_nozzle to use K=0 where the two models are designed to agree; with K losses the models correctly diverge (equivalent-length vs lumped-Cd approximations). After fix: _fanno_state K=0: mdot=0.000429 (matches issue table 0.000427) _fanno_state K=1.5: mdot=0.000314 (matches issue table 0.000313) ratio Fanno/lossy = 0.851 < 0.92 (acceptance criterion met) Monotonicity: PASS across all pressure ratios All 58 tests pass https://claude.ai/code/session_019hMsyJv1g6gSZF3h9SpFrB
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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
2 participants
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.
Summary
This PR refactors the Fanno flow calculation in the venting module to improve accuracy and consistency. The main changes involve removing the short-tube approximation path for certain pressure ratios and correcting the mass flow rate calculation to use static pressure instead of stagnation pressure.
Key Changes
Removed short-tube approximation: Deleted the conditional branch that applied
mdot_short_tube_pos()when downstream pressure ratio exceeded 0.6. The Fanno tube model now handles all pressure ratios consistently.Fixed mass flow rate calculation: Changed the mdot calculation to use static pressure
p1instead of stagnation pressureP_up. This corrects the thermodynamic basis of the calculation:mdot = P_up * m1 * a_eff * sqrt(...)mdot = p1 * m1 * a_eff * sqrt(...)Removed pressure loss factors from outlet pressure: Simplified the
outlet_pressure()function by removing the application ofK_inandK_outloss coefficients. The function now returns the isentropic pressurep2directly instead of applying lumped loss corrections.Simplified final mdot calculation: Removed the ad-hoc pressure-ratio-dependent correction that was clamping mdot to
0.99 * md_lfor low pressure ratios. The calculation now uses the consistent formula throughout.Code cleanup: Extracted intermediate calculations (
t1_final,p1_final) for clarity and to avoid redundant computations.Test Updates
test_fanno_low_mach_matches_lossy_nozzle()to useK_in=0, K_out=0for the agreement check, with clarifying comments explaining why loss coefficients must be zero for model equivalence.test_fanno_with_K_below_lossy()assertion from< 1.0to< 0.92to reflect the new calculation's behavior.test_fanno_monotonic_with_K()tolerance from0.98 * prevtoprevto account for improved numerical behavior.Implementation Details
The refactoring moves away from a hybrid approach (short-tube for high pressure ratios, Fanno for low) to a unified Fanno tube model. The use of static pressure in the mass flow calculation is more thermodynamically consistent with compressible flow theory, where mass flow depends on local conditions at the throat/reference section rather than upstream stagnation conditions.
https://claude.ai/code/session_019hMsyJv1g6gSZF3h9SpFrB