Repository navigation
Add the F404 test suite (#5), and the restructuring it needed - #15
Merged
Merged
Conversation
Removes two src/ files that no longer earn their place, ahead of building the real test suite (#5): - run_design_od.py — its docstring states its purpose as "preserves the original MFTF_od_CRZ.py behavior for regression testing", but that monolith was deleted in b96c02e. The regression-checking role it was holding open is exactly what the golden-baseline tests in this branch take over, and it exposed no importable functions — the whole file was one `if __name__ == "__main__"` block. - test_modes.py — a 181-line smoke script with zero assertions that runs a 4-point solve at import time. Beyond being superseded, the name is a hazard: pytest collects test_*.py, so leaving it in place means every test session would execute two full DESIGN+OD solves during collection. Between them they carried a third and second copy of the design-point / OD initial-guess setup that also lives in sweep_full_envelope.py; the next commit consolidates the surviving copy. Also tightens .gitignore. `/MFTF_od_CRZ_out/` only covered the one long-deleted script, so every other OpenMDAO reports directory (problem_out/, run_design_od_out/, sweep_full_envelope_out/) was showing up as untracked noise — widened to `*_out/`. Stray cycle_deck_*.csv files dropped in a working directory by a sweep are ignored too; reviewed decks live in deck/ and are added deliberately. Dropped the line that had .gitignore ignoring itself, which did nothing since the file is tracked.
The modules under src/F404_pycycle/ imported each other by bare module name (`from mp_cycle import MPMixedFlowTurbofan`), which only resolves when the interpreter's working directory is that folder — i.e. only when a file in it is run directly as a script. Nothing outside could `import F404_pycycle.sweep_utils`, so a test suite had no way to reach the code under test. That is the blocking prerequisite for #5. - Intra-package imports become absolute (`from F404_pycycle.mp_cycle import ...`). - setup.py grows an `F404_pycycle` entry plus a package_dir mapping to src/F404_pycycle. IMPROVEMENTS.md item 1 floated a second setup.py for the app code; folding it into the existing distribution instead keeps the documented `pip install -e .[all]` as the single install step, and the two package trees stay namespaced apart regardless. - pytest config in pyproject.toml: pythonpath=["src"] so the suite runs from a fresh clone without an editable install (CI shouldn't depend on install mode), testpaths=["tests"] so collection doesn't wander into the vendored pycycle/ suite that testflo owns, and python_files=["*_test.py"] for the per-module naming from #5. - pytest>=7 added to the `test` extra (the pythonpath ini option is 7+). Entry points are now invoked as `python -m F404_pycycle.sweep_full_envelope`; README usage and the repo-layout table updated to match, and the table drops the two files deleted in the previous commit.
The design-point targets, component efficiencies and Newton initial guesses existed in three near-identical copies — sweep_full_envelope.py, test_modes.py and run_design_od.py. Two of those were deleted as dead scripts; this lifts the surviving copy out of the sweep driver into `problems.py` as `build_dry_problem()` / `build_wet_problem()`, so the sweep CLI and the test suite construct the engine the same way rather than drifting apart the way the three scripts already had. Besides deduplication this buys three things the tests need: - `verbose=False`. The builders previously always printed Newton iteration traces and two full page_viewer dumps; an integration test that emits ~200 lines of tables per case is unusable. - Fail-fast validation. `_validate_design_targets()` rejects non-finite targets (which otherwise pass silently through `prob.set_val()` into the residual), a non-positive `fn_target` (which pins the DESIGN W balance at its 25 lbm/s lower bound and sizes a nonsense engine that every downstream OD point then inherits), and `dsn_Tt7 <= mil_Tt4` (asking the afterburner to cool the flow, which pins FAR_ab at 1e-4). Each raises a ValueError naming the value, the balance it breaks, and the expected range — these are the mistakes that currently surface only as an opaque Newton divergence minutes later. - Parameterised targets. `fn_target`/`mil_Tt4`/`dsn_Tt7` are arguments with the module constants as defaults, so tests can drive off-nominal sizings without editing source constants. Also drops a redundant second `run_model()` per builder. The original "verify OD at design conditions" block re-set the same four OD values that were already set before the first solve, then solved again — the first `run_model()` already runs DESIGN and OD together, so the second pass re-converged an already-converged point purely to print its page. Verified unchanged after the consolidation: dry DESIGN Fn = 11,000 lbf, W = 144.1825 lbm/s, BPR = 0.7528; wet DESIGN Fn = 17,700 lbf, W = 137.0927 lbm/s; both OD points reproduce design thrust. The dry/wet sizing-divergence TODO moves onto the problems.py module docstring, pointing at #2 rather than restating it.
First tests under #5's `<module>_test.py` convention, placed in a top-level tests/ directory rather than as src/ siblings so the package stays model code only. 26 tests, all sub-second — the grid builders are pure functions, and SweepRunner's guards only read scalars off the problem, so a StubProblem exercises them without a solve. The guards are the point of this file. The bug fixed in 51c9bb6 was cycle decks full of rows Newton called converged that were nothing of the sort: BPR clipped to 1.0, LP spool parked on its 500 rpm floor, Fn > 100,000 lbf. `_state_at_bounds` and `_target_met` are what stand between that and the deck now, and until this commit neither had a single test. Covered: each balance saturating at each bound, the 1% bound-width tolerance band (197.0 lbm/s is a real high-flow solution, 198.5 is pinned), FAR_ab saturation correctly ignored in dry mode where no such balance exists, missing states skipped rather than failing a point, and wet mode requiring *both* Tt4 and Tt7 on target — a drifted core invalidates the row even at the right T7, since the deck would otherwise label a point mil power that isn't. Also locks `_OD_BOUNDS` against the bounds engine_model.py actually declares, by reading them back off the built BalanceComp. That table is a hand-maintained second source of truth — its own docstring says it "must mirror engine_model.py" — so editing a bound in the model would otherwise leave the saturation check silently measuring against the old one, with no symptom other than bad rows reappearing in the deck.
Covers problems.py with the two remaining categories #5 asks for. Fail-fast (instant — no model is built): non-positive and non-finite thrust targets, non-positive Tt4, and a wet augmentor target at or below the core target. One test asserts on the message content rather than just the exception type, since a ValueError that doesn't name the bad value, its units and a sane range is barely better than the Newton divergence it replaces. Golden baselines, captured on this branch at 1e-4 relative tolerance — tight enough to trip on any real cycle change, loose enough to survive last-digit BLAS differences across the CI platforms: dry DESIGN Fn 11,000 lbf W 144.1825 lbm/s BPR 0.7528 TSFC 0.6201 wet DESIGN Fn 17,700 lbf W 137.0927 lbm/s BPR 0.7528 TSFC 1.5245 FAR_ab 0.0415 This is the safety net the handoff wanted in place before #2/#3/#8 — all three are Newton-solver surgery on a model whose behaviour is currently known-good and entirely unguarded. Beyond the numbers, some assertions are structural and survive a re-baseline: OD run at design conditions must reproduce the DESIGN solve (checks the map-scalar and station-area handoff in mp_cycle.py), OD spool speeds must come back to the 10,000/14,000 rpm the engine was sized at (independent check on the shaft power balances, since Nmech is an input at DESIGN and a solved state at OD), mixer ER must close on 1.0 (the DESIGN BPR balance's only job), and max AB must cost more than 2x mil power's TSFC. Also measures the #2 divergence the handoff listed as its next step. Dry and wet agree exactly on everything dimensionless — same PRs, same Tt4, same BPR — but their DESIGN mass flows differ by 5.17% (144.1825 vs 137.0927 lbm/s), not the 1-2% the issue estimates. Recorded as an explicitly-labelled characterisation test that should fail and be deleted when a single sizing serves both modes.
Writing the tests surfaced 59 DeprecationWarnings per run, all of the same kind: `prob.get_val()` and `prob[...]` return length-1 ndarrays, and `float()` on an array with ndim > 0 has been deprecated since NumPy 1.25 and is slated to raise. sweep_utils.py does exactly that ~18 times per sweep point in extract_od_results, plus once per balance in _state_at_bounds and twice in _target_met — so on a future NumPy every one of those becomes a TypeError, and in _state_at_bounds and _target_met the bare `except Exception` around the read would swallow it and quietly report the point as unconverged. Adds `_scalar()` and routes every value read through it. This is the same NumPy 2 compatibility class the CI workflow already gates on with `ruff check --select NPY201`, which doesn't flag this particular pattern. Incidentally makes the module's `import numpy as np` load-bearing — it had no other use. pytest promotes this specific deprecation to an error so a bare float() can't reappear unnoticed.
Two files of build-but-don't-solve tests, 24 in total, all in ~3s. engine_model_test.py locks in the structural decisions that cost the most to arrive at and are the easiest to silently undo: - Dry mode substitutes a pyc.Duct for the afterburner rather than a zero-FAR Combustor. The Combustor form is rank-deficient — every output equation collapses to a copy of its input — and crashes the OD linear solve at part-power dry conditions (51c9bb6). - err_on_non_converge stays True. With it off, Newton returns whatever stalled or bound-clipped state it reached and the sweep writes it to the deck; that is what produced decks with Fn > 100,000 lbf. - The linesearch stays ArmijoGoldsteinLS at maxiter=0, the fast path SweepRunner raises only on a per-point retry. - DESIGN solves turbine PRs with spool speeds given, OD does the reverse. Swapping them yields an over- or under-determined model. - F404 architecture: no LPC, both shafts at num_ports=2, both cooling bleeds wired to their turbines, CD nozzle. mp_cycle_test.py covers the design-to-off-design handoff, where a dropped connection is near-invisible — the OD point just solves a slightly different engine and writes a plausible number to the deck. All 16 map scalars and all 17 frozen station areas are asserted to reach OD, plus the throat-area-to-W-balance link that #3 and #8 both hinge on. Connections are matched on prefix and suffix rather than exact path, since OpenMDAO resolves them down to pyCycle-internal subcomponents (DESIGN.fan.map.scalars.s_PR) that an upstream refactor could rename without anything here actually changing. Also asserts T7 is settable per point — if rhs:FAR_ab were promoted to a cycle parameter it would be shared across points, pinning OD to the design T7 and making the wet throttle sweep impossible. mp_cycle.py has a comment saying so; now something enforces it.
Closes out the coverage. 94 tests, 18s for the full suite. sweep_full_envelope_test.py pins the per-column deck precision that #12 established (PR #14). The decks are the repo's product and are committed for review, so a formatting change is a diff across every row of every file — worth locking exactly: scientific %.4e for the fuel-air ratios, 4 decimals for the continuous quantities a downstream optimizer reads, 2 elsewhere. Two of these guard the generic behaviour rather than the current column list: no numeric column may escape formatting (a new column added to extract_od_results falls through to the 2-decimal default rather than full float64 repr, which is what #12 set out to stop), and the writer must not reformat the caller's DataFrame in place — the combined deck is written from a concat of the per-mode frames that were already written. Two end-to-end sweeps in sweep_utils_test.py exercise the real loop — solve, guard, extract, collect. The happy path runs three SLS points and asserts the full deck schema comes back, T4 lands on target for every row, and both mass flow and thrust fall monotonically as the day heats up. The other feeds an unreachable 800 degR power target — below compressor discharge temperature, so the burner would have to remove heat — and asserts the deck comes back *empty*. That is the shape of the original bug: the sweep used to return a row for a point like this, populated with whatever state Newton stalled in. Both build their own problem rather than taking the session fixture, since running a sweep mutates OD solver state.
printer.py sat at 9% coverage — the one module nothing else exercises, because every other test reads the model programmatically while the printer hardcodes a list of station and component names. Rename a station in engine_model.py and the physics stays correct while every interactive run dies in a KeyError. Covering it needed a small production change. pyCycle's print_* helpers take `file` but bind `sys.stdout` as a *default argument*, evaluated at import — so their output escapes both capsys and capfd and there is no way to assert on it. printer.py now threads a `file` parameter through page_viewer and print_perf, resolving None to sys.stdout at call time rather than repeating the same late-binding mistake. Tests render into a StringIO and assert every table heading is present, in both dry and wet configurations (the afterburner is a Duct in one and a Combustor in the other, and both are asked for a burner table). Adds .github/workflows/f404_test_workflow.yml, separate from the vendored library's testflo workflow. Runs pytest with coverage on ubuntu py3.9 / py3.12 and macos py3.12 — the suite asserts on converged solver output, so it is worth knowing early if a NumPy or SciPy version moves the numbers out of the 1e-4 band. Coverage is now 86% of src/F404_pycycle (98 tests, ~25s): mp_cycle.py 100% printer.py 100% engine_model.py 98% (a dead USE_TABULAR=False branch) problems.py 93% (verbose print path) sweep_utils.py 83% sweep_full_envelope.py 37% The last one is the floor on this number: everything uncovered there is inside `if __name__ == "__main__"`, which can't be imported, let alone called. Extracting it behind a real entry point is the CLI work.
This was referenced Sep 20, 2026
- README gains a Testing section (how to run, the slow marker, the 86% coverage figure and why sweep_full_envelope.py caps it), and the roadmap moves #5 to Done. The mermaid data-flow diagram drops the deleted run_design_od.py and gains problems.py, and the layout table gains problems.py and tests/. - IMPROVEMENTS.md items 2, 3 and 5 get status banners rather than a rewrite — it's a planning record, so the original prose stays and the banners say what actually happened, including the one deviation (tests in a top-level tests/ directory, not as src/ siblings). - The handoff gets a session summary: the restructuring the suite needed, the golden baseline table, the decisions a future agent would otherwise have to re-derive (why connections are matched on prefix+suffix, why printer.py grew a `file` parameter), and the two findings — #2's divergence measuring 5.17% rather than the filed 1-2%, and the latent NumPy 2 break in sweep_utils.py. Filed two issues while here: #16 (proper CLI entry point, absorbing #13's min,max,step alt/Mach/throttle flags) and #17 (the pydantic refactor IMPROVEMENTS.md item 3 deferred until the src/ layout settled — its preconditions are now met).
CI failed on all three matrix jobs with ModuleNotFoundError: No module named 'pandas' at collection — 4 test modules x 3 jobs, one cause. Not a test bug. `install_requires` has only ever listed openmdao, but the F404 code has always imported pandas directly: SweepRunner collects sweep results into a DataFrame and write_deck_csv serialises the deck from it. A clean `pip install -e .[all]` has therefore never produced a working sweep — it worked locally only because pandas was already in the environment. The new CI job is the first thing to install this repo from scratch and actually import the application code, so it's the first thing to notice. numpy is declared alongside it. It resolves transitively through openmdao, which is why nothing broke, but sweep_utils.py and sweep_full_envelope.py import it directly and shouldn't depend on another package's dependency graph to keep doing so. Dropped the unused `import numpy as np` from engine_model.py, the one module that imports it without using it.
The filterwarnings rule added alongside the _scalar() fix promoted
"Conversion of an array with ndim > 0 to a scalar" to an error for the
whole pytest session, not just this package. The vendored pycycle/
library triggers that same deprecation throughout its own numerics —
pycycle/thermo/cea/props_rhs.py assigns a length-1 array into a scalar
slot, for one — so pointing pytest at pycycle/ turned upstream's
warnings into hard failures:
pytest pycycle/elements/test/test_bleed_out.py
before: 1 failed (ValueError: setting an array element with a sequence)
after: 1 passed (198 warnings)
testpaths=["tests"] meant a bare `pytest` never hit this, but an
explicit path overrides testpaths, so it was one command away.
Appending the `F404_pycycle.*` module pattern scopes the rule to the
code it was written to guard. Verified the guard still bites: restoring
a bare float() in sweep_utils._scalar fails 15 tests.
Upstream's deprecations are a vendor-sync concern, not something this
suite should be enforcing on another project's code.
CI caught two genuine robustness bugs in SweepRunner that the local run didn't, because the platform changes how a degenerate point fails. **1. A failed point could abort the sweep entirely.** `_run_point` caught only `om.AnalysisError`. On CI's scipy the unreachable-target point doesn't exhaust Newton — it collapses the Jacobian underneath it, and DirectSolver raises `RuntimeError: Jacobian in 'OD' is not full rank`, which propagated straight out of `run_sweep`. A sweep is hundreds of points and takes hours; losing all of them to one corner the deck was never going to cover is its own failure mode. Now caught alongside AnalysisError, and logged at debug rather than swallowed silently. **2. A failure before the first success poisoned everything after it.** With #1 fixed the sweep survived, but every subsequent point then failed too. `_last_good_state` is only populated after a point converges, so an early failure had nothing to restore from — the model stayed in whatever state Newton abandoned and every later point warm-started from it. `run_sweep` now seeds the fallback from the state it was handed, which is OD converged at design conditions (see problems.build_dry_problem / build_wet_problem). The second is latent on the current grid rather than theoretical: it only stays hidden because the first point of both the dry and wet sweeps happens to converge. Any grid reordering, or a wider envelope from #16/#13, would surface it as a near-empty deck with no obvious cause — the failure looks like "the model broke", not "point 1 failed". Both are exactly the class of thing this suite is for, and neither was reachable before there was a test that deliberately fails a point. Coverage of sweep_utils.py rises to 89%.
Owner
Author
CI triage
1. 2. One bad sweep point could abort the entire sweep ( 3. A failure before the first success poisoned every point after it ( #3 is latent rather than theoretical — it stays hidden only because the first point of both the dry and wet grids happens to converge. Any grid reordering, or the wider envelope from #16/#13, would surface it as a near-empty deck with no obvious cause: the failure looks like "the model broke", not "point 1 failed". Neither #2 nor #3 was reachable before there was a test that deliberately fails a point. Coverage now 87%. Also fixed: the The remaining red check is not from this PR
That workflow had simply never run before — it triggers on pushes to Filed as #18. |
The three pre-existing bugs the new workflow exposed (undeclared pandas dependency, a sweep-aborting RuntimeError, and the unseeded _last_good_state that poisons a sweep after an early failure), plus two things a future agent would otherwise rediscover the hard way: why the NumPy deprecation filter must stay scoped to F404_pycycle, and that the red pyCycle check is a vendored-library incompatibility with OpenMDAO 3.45.1 tracked in #18, not something this branch introduced. Calls out that the _last_good_state bug is latent rather than fixed-and- forgotten: it only stayed hidden because the first point of both grids converges, which is exactly the assumption #3/#8 will be perturbing.
Jhawk414
marked this pull request as draft
September 20, 2026 03:41
Takes pycycle/elements/test/test_bleed_out.py from upstream OpenMDAO/pyCycle master (da3b5e3, "Modernize CI to use pixi envs from OM. Also, fix tests to pass with more recent OM"), which is the last red check on this PR. The vendored copy fails on OpenMDAO 3.45.1 — what CI installs, since pycycle_test_workflow.yml pins OPENMDAO: 'latest' — because BleedOut promotes four inputs to bleed.Fl_I:tot:T with mismatched units (three degK, one degR). Older OpenMDAO resolved a get_val(..., units='degR') against that; 3.45.1 raises and asks for an explicit set_input_defaults. Upstream's fix is precisely the two lines the error message suggests: cycle.set_input_defaults('bleed.Fl_I:tot:T', units='degR') cycle.set_input_defaults('bleed.Fl_I:tot:P', units='psi') Test-only — no production pycycle code changes, so nothing the F404 model depends on moves. Taken as the whole file rather than a hand- applied hunk so the vendored copy matches upstream byte for byte and the next sync sees no local divergence here. Still passes on 3.39.0 locally, so this isn't trading one version's green for another's red. Resolves #18.
Replaces the "known-red check" section, which is no longer accurate now that all 7 checks pass. Keeps the more durable point for whoever picks up vendor sync: the pyCycle workflow had never run before this PR, and the first time it did it found a failure upstream had already fixed. #6 was closed on one spot check (the NumPy 2 .item() fix being present); this is evidence the vendored tree is behind in ways nothing is currently watching, and that a real file-by-file diff against upstream is still worth doing.
Scopes pycycle_test_workflow.yml's push and pull_request triggers to `pycycle/**` (plus the workflow file itself). Four jobs at 2-5 minutes each, testing upstream code that F404 application work never touches, was pure latency on every F404 PR — and the F404 suite already covers this repo's own code in ~2 minutes. Safe to skip: `main` has no branch protection, so a workflow that doesn't run can't leave a PR stuck on a required-but-pending check. setup.py is deliberately not in the path list. A packaging change can in principle break the pycycle install without touching pycycle/ — the tradeoff is noted in the workflow, and workflow_dispatch is still there with its full job-selection inputs for running it manually. This PR still triggers it, since it changes a file under pycycle/ — which is the point: the cherry-picked test_bleed_out fix gets verified before the trigger narrows for everything after.
Jhawk414
marked this pull request as ready for review
September 20, 2026 04:25
Owner
Author
GPT review:Reviewed PR #15, the README, and relevant issues. Overall, the suite is substantive—not test-count padding. The golden solves, DESIGN→OD wiring, convergence guards, and failed-point recovery tests are worthwhile.
|
Review found the convergence guards were tolerant in ways that defeat their own purpose, and that one test had frozen that tolerance in place. **_state_at_bounds no longer skips an unreadable state.** It caught every exception and `continue`d, so a balance renamed in engine_model.py would silently stop being checked — bound-clipped solutions sailing through as converged, which is precisely the failure the guard exists to catch, and with no symptom. Now raises SweepConfigurationError naming the state, the table to update and the file it mirrors. Every key in _OD_BOUNDS is present in every OD configuration (FAR_ab is added only when it exists), so there is no legitimate miss to tolerate. The test asserting the old skip behaviour was endorsing the gap; it now asserts the raise instead. **Narrowed the blanket excepts.** `_run_point` catching RuntimeError is load-bearing — that is how DirectSolver reports a singular Jacobian, and one hard corner must not cost a multi-hour sweep — but it was logging at debug, so the only record of a non-Newton failure was invisible by default. Now logged at warning. `_run_bridge`'s `except Exception` is narrowed to the same two types and likewise logged. SweepConfigurationError is deliberately not a RuntimeError, so a drifted bounds table travels through that except clause rather than being recorded as an ordinary failed point. Two tests cover the boundary: an unexpected KeyError aborts the sweep, and so does a drifted bounds table. **The #2 test now asserts the fix, not the bug.** It measured the 5.17% dry/wet divergence, which made it fail when the product improves. Split: the dimensionless agreement (same PRs, same Tt4, same BPR) is a real contract and stays a normal test; the sizing equality becomes xfail(strict=True), so fixing #2 turns it into an XPASS that fails the suite until the marker is removed. Measurement moved to issue #2, where it is durable and doesn't gate CI. **Dropped test_module_constants_are_self_consistent** — it restated four adjacent literals and duplicated the finite-value validation. **Warning suppression actually works now.** The filterwarnings calls in conftest.py did nothing: pytest re-applies its own configuration around each test, overriding module-level filters, so the "expected warnings would bury real output" comment sat above 81 warnings per run. Moved to pyproject.toml, ordered so the blanket ignores come first and the targeted NumPy deprecation error still overrides them. Run is now clean; verified the deprecation guard still bites (15 failures on a restored bare float()). 101 passed, 1 xfailed, 0 warnings. Coverage 87%.
98 tests / 86% was stale — the suite is 102 (101 passing, 1 xfail tracking #2) at 87%, and the full run is ~35 s rather than ~25 s now that it includes the error-propagation cases. Flagged by review. Also updates the handoff's description of the #2 measurement to match what the suite now does: an xfail against the desired equal-sizing behaviour, rather than a characterisation test asserting the defect.
Owner
Author
Response to reviewGood review — it caught a real inversion in two of the guards. Agreed with five findings and pushed fixes in Agreed and fixed1. 2. 3. The #2 test protecting the bug — agreed, and the 4. 5. Stale docs — agreed, fixed. README / handoff / IMPROVEMENTS now say 102 tests, 87%, ~35 s. 6. Ineffective warning suppression — agreed, and the diagnosis was exactly right: pytest re-applies its own config around each test, so Partially disagreedCatching all The concern is that unrelated programming errors get downgraded to "point failed." In practice they don't, because programming and configuration mistakes don't raise Matching only the identifiable singularity means matching message text ( What was genuinely wrong: it logged at I did take the test, in both directions: an unexpected One deliberate asymmetry worth naming: |
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
1 participant
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
Implements #5 — a pytest suite over
src/F404_pycycle/, plus thesrc/cleanup and packaging work that had to happen first. 98 tests, ~25 s, 86% coverage.The handoff put #5 ahead of all the solver work (#2, #3, #8) for a reason: each of those is Newton surgery on a model whose behaviour is currently known-good and entirely unguarded, and the last round of that work produced cycle decks full of rows Newton called converged that were nothing of the sort. This is the net.
Three things blocked writing a single test, and are fixed here:
from mp_cycle import ...), which only resolves when the interpreter's cwd is that folder — i.e. only when a file in it runs as a script. Now absolute imports, withF404_pycycleregistered insetup.py. Entry points are invoked aspython -m F404_pycycle.sweep_full_envelope.test_modes.pywould have detonated on collection. 181 lines, zero assertions, running a 4-point solve at import — and named so pytest collects it. Deleted, along withrun_design_od.py, whose docstring pinned it to a monolith deleted back inb96c02e.problems.pyasbuild_dry_problem()/build_wet_problem(), which also gets the suite averbose=False, parameterised targets, and somewhere for fail-fast validation to live.What the tests actually guard
Most of the value is in the regression surface around false convergence — the
51c9bb6bug class, where a bound-clipped or stalled state gets written to the deck as a real row:err_on_non_converge=Trueand theArmijoGoldsteinLS(maxiter=0)default are asserted structurallySweepRunnerguard is unit-tested against a stub: each balance saturating at each bound, the 1% bound-width tolerance band,FAR_abcorrectly ignored in dry mode, and wet mode requiring both Tt4 and Tt7 on target_OD_BOUNDSis locked against the boundsengine_model.pyactually declares, by reading them back off the builtBalanceComp— that table is a hand-maintained second source of truth and its own docstring admits itGolden DESIGN/OD baselines at 1e-4 relative tolerance:
Plus structural assertions that survive a re-baseline: OD run at design conditions must reproduce the DESIGN solve (checks the map-scalar/station-area handoff), OD spool speeds must return to the 10,000/14,000 rpm the engine was sized at, mixer ER must close on 1.0, all 16 map scalars and 17 frozen station areas must reach OD.
Two findings worth surfacing
sweep_utils.pycallsfloat()on OpenMDAO's length-1 arrays ~18 times per sweep point; that's been deprecated since NumPy 1.25 and is slated to raise. Worse, in_state_at_boundsand_target_metthe surroundingexcept Exceptionwould swallow the eventualTypeErrorand silently report every point as unconverged. Fixed, and pytest now promotes that deprecation to an error. This is the same class the CI already gates on withruff --select NPY201, which doesn't catch this pattern.Coverage
mp_cycle.pyprinter.pyengine_model.pyUSE_TABULAR=Falsebranchproblems.pysweep_utils.pysweep_full_envelope.pyif __name__ == "__main__"That last row is the ceiling on the total: an uncallable
__main__block can't be tested. Extracting it behind a real entry point is the CLI work.Deviations from the issue as filed
tests/, not assrc/siblings. Add regression/test suite convention (per-module <module>_test.py) #5 andIMPROVEMENTS.mditem 2 specify<module>_test.pynext to each module. The naming convention is kept; the location isn't, sosrc/stays model code only and test fixtures don't ship inside the package.IMPROVEMENTS.mditem 3 already scopes that as its own branch. Validation here is plainValueErrorwith messages that name the offending value, its units, the balance it breaks and a sane range. Filed separately.Related Issues
Backwards incompatibilities
Entry points move from
python src/F404_pycycle/sweep_full_envelope.pytopython -m F404_pycycle.sweep_full_envelope.run_design_od.pyandtest_modes.pyare deleted (recoverable from history). README updated.New Dependencies
pytest>=7.0andpytest-cov, both in the existingtestextra — not runtime dependencies.