Repository navigation
Conversation
The two FDTD simulation functions and both run() methods now write every frame and probe sample into one array allocated before the march, and the four largest results (both FDTD simulations and both parabolic equations) hand that array to their record, which seals it read-only and keeps it instead of copying it. The private handed_over keeps an array only if it owns its memory, is C-ordered and reaches its first record; anything else is copied as before. make array-aliasing gains a part that proves from the code that nothing else can reach an array handed over, with no exemption. Every result is bit for bit what it was, and peak memory falls from 855 to 322 MB, 287 to 128 MB, 333 to 202 MB and 139 to 111 MB on the measured runs.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (14)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review. 📝 WalkthroughWalkthroughThe change adds ChangesArray handoff and simulation results
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Refactor Sequence Diagram(s)sequenceDiagram
participant fdtd_simulation
participant _record_run
participant FDTDResult
participant _owned_array
fdtd_simulation->>fdtd_simulation: Allocate pressure history and optional snapshots
fdtd_simulation->>_record_run: Pass arrays for in-place recording
_record_run->>_record_run: Write initial field and scheduled frames
fdtd_simulation->>FDTDResult: Pass arrays through handed_over
FDTDResult->>_owned_array: Check arrays for adoption
Merge Risk: ⚪ Minimal · up to The change cuts peak memory for large simulation results, and the author reports no public output changes. I found no concrete merge-blocking risk in the supplied context. Normal CI should still run before merge. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The reviewed factories retain caller isolation while reducing peak memory use. No introduced security defect was established, but failure recovery and concurrent handoff guarantees are not fully demonstrated. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 69.49% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 59 functions across 10 files. (4 skipped: 4 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
documentation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## fix/regimes-derive-from-their-values #954 +/- ##
=====================================================================
Coverage 96.81% 96.81%
=====================================================================
Files 443 443
Lines 89035 89043 +8
=====================================================================
+ Hits 86200 86211 +11
+ Misses 2835 2832 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Numerical conformance
1893/1893 checks pass across 111 domains and 523 standards (227 normative designations, 125 further published sources). Used in the tables below is how much of that clause's published tolerance the deviation consumes: 100 % sits exactly on the limit, 5 % uses a twentieth of the allowance, and a dash means the clause states no two-sided tolerance for the quantity, so there is no budget to spend. It is reported and never used to decide a verdict, which is settled at full precision before any rounding. Nothing moved: same 1893 checks, same verdicts, same numbers. Closest to their published limit (top 5) The rows with the least room left, so the ones a change is most likely to push over.
|
| Python Version | Tests | Failures | Coverage | Status |
|---|---|---|---|---|
| macos-latest-3.13 | 27270 | 0 | 96.8% |  Passed |
| macos-latest-3.14 | 27270 | 0 | 96.8% | Passed |
| ubuntu-latest-3.13 | 27270 | 0 | 96.8% | Passed |
| ubuntu-latest-3.14 | 27270 | 0 | 96.8% | Passed |
| windows-latest-3.13 | 27270 | 0 | 96.8% | Passed |
| windows-latest-3.14 | 27270 | 0 | 96.8% | Passed |
Full report at this commit: docs/CONFORMANCE.md · docs/conformance.json · full CI artifacts
|
This branch has not been deployed
1 participant
Since every public record keeps a read-only copy of its own of the arrays it holds, the largest results paid for that copy in memory: while the record was being built, the solver's arrays and the record's copy were both alive. This change takes that cost away again for the four results where it was large, without weakening the rule. A record still holds read-only arrays that nothing else can reach, and a record you build by hand still copies what you give it.
simulation.fdtd_simulationandsimulation.elastic_fdtd_simulationgathered their snapshots in a list, stacked the list into a second array and handed that to the record, which copied it once more, so a run held its frames three times at its peak. Each now writes every frame, and every probe sample, into one array allocated before the march with the exact number of frames.FDTD2D.runandElasticFDTD2D.run, which held their frames as a list and its stack, write into one array the same way; what they return stays a writeable array, as before.The two simulation functions then hand that array to their record, which seals it read-only and keeps it instead of copying it.
underwater.parabolic_equationandenvironment.atmospheric_parabolic_equationalready wrote their field range by range into one array, so for them the record's copy was the whole extra cost, and they hand their field over the same way. This goes through a private function,handed_overinphonometry._internal.frozen. The record keeps such an array only if it owns its memory and is C-ordered, and only the first record it reaches keeps it; anything else is copied as before, so every record holds the same layout however it was built.Keeping an array without a copy is only right when nothing else can reach it, so
make array-aliasingand its CI job gain a part that proves this from the code for everyhanded_overcall, or fails, with no exemption. The call has to be an argument of a record that copies its arrays, built in thereturnstatement. Its argument has to be a name bound once, in the function and to it alone, tonp.zeros,np.emptyornp.ones(orNonein one branch of a conditional), in a function that is no generator and does not hand out its frame (locals(),vars(),eval,exec). Every other read of the name has to write into the array, test it againstNone, or pass it to an undecorated helper of the package whose parameter is used the same way, three calls deep at most. A view, a second name, a container, a closure or a lambda rules it out. The function is found however it is spelt: imported under another name, read through a dotted path to its module, or fetched by its name as a string.Peak memory, measured with
tracemallocon the same runs before and after:Before results copied their arrays at all, the same runs peaked at 587, 207, 202 and 112 MB, so the two FDTD runs now need less memory than they did before that change too.
What breaks. Nothing: no public name, signature, default or value changes, every result is bit for bit what it was, and the arrays a result holds were already read-only. There is nothing to migrate. For contributors, CONTRIBUTING section 7f says when a factory may hand an array over and what the gate proves of each call, and the Makefile and CI comments describe the gate's new part.
How it was verified. Every array of fifteen runs (small and benchmark-sized FDTD and elastic FDTD runs with and without snapshots, each snapshot field,
run()on both engines, both parabolic equations) was compared with the same runs before this change, dtype, shape and bytes, withnp.array_equaland the writeable, C-contiguous and own-data flags: no difference. New tests pin each snapshot frame to the field of its own step: at a probe's cell, frame k equals, bit for bit, what the probe recorded k cadences in, on runs that are not a whole number of cadences long, for the acoustic FDTD and for the elasticp,vxandvyfields. A frame index one cadence off fails them. tests/test_owns_arrays.py (57 tests, 15 new) checks that an array handed over is kept and sealed, that a second record copies it, that a view (C-ordered or not) or a Fortran-ordered array is copied and the caller's view keeps itswriteableflag, and withtracemallocthat none of the four functions and neitherrun()holds its result twice; putting back the list and its stack fails them. tests/test_check_array_aliasing.py (116 tests, 35 new) has the gate accept the factories' shape and refuse each way the array could be reached otherwise, a decorated helper and a dotted path to the function included, and the gate was seen red on a scratch copy of the tree with a snapshot array kept on the engine and with a view of a parabolic-equation field handed over. The full suite passes on Python 3.13 (27201 passed, 69 skipped), the conformance report is unchanged (1893 of 1893 checks), the API reference and llms files regenerate with no change, the related check scripts pass, and ruff, ruff format, mypy (689 files) and bandit are clean. No documentation code, figure or site page changed.Summary by CodeRabbit
Performance
Behavior
run()remains writable.