Add WC stability metrics, live drift monitor, sandbox configs, and locked geometry generator - #359
Add WC stability metrics, live drift monitor, sandbox configs, and locked geometry generator#359lsmolinski wants to merge 15 commits into
Conversation
Signed-off-by: lsmolinski <l_smolinski@o2.pl>
Signed-off-by: lsmolinski <l_smolinski@o2.pl>
Signed-off-by: lsmolinski <l_smolinski@o2.pl>
Fix encoding and filename in the K-selectivity sandbox Strip a UTF-8 BOM from electron_k10_vmode10_soft_pressure_00001.py and rename "electron_k10_vmode10_soft_pressure_0001 (2).py" to electron_k10_vmode10_soft_pressure_0001_2.py: the space and parentheses break shell tooling on the file list. Run black over the PR's changed files (line-length 99) Signed-off-by: Rodrigo Griesi <56765588+xrodz@users.noreply.github.com>
xrodz
left a comment
There was a problem hiding this comment.
Summary
This adds a wave-centre stability subsystem (mean pairwise drift plus active count, a live panel and post-run plots), moves the K-selectivity configurations into xparameters/sandbox_k_selectivity/, and adds a lock-in-scaled 1-3-6 geometry generator alongside the existing one.
Verdict: changes requested. Four blocking findings, all fixable here. The new geometry is a genuine improvement on what it sits beside, and the measurements below say so.
Blast radius
| Tier | Paths | Standard applied |
|---|---|---|
| T1 model-local | all 36 files, inside openwave/xperiments/m4_ewt/ |
light review |
| T3 shared code | none | |
| T4 shared surfaces | none |
Nothing reaches outside M4, so this is a light review by the blast-radius map.
Maintainer edits already on this branch
Per dev_docs/PR_REVIEW_STANDARDS.md § 10, mechanical fixes get made rather than asked for, and announced:
| Edit | Detail |
|---|---|
| Stripped a UTF-8 BOM | electron_k10_vmode10_soft_pressure_00001.py started with one, 2122 to 2119 bytes |
| Renamed a file | electron_k10_vmode10_soft_pressure_0001 (2).py to electron_k10_vmode10_soft_pressure_0001_2.py. The space and parentheses break shell tooling, including the file-list run used in this review |
| Ran black | 28 of the 36 changed files, line-length 99 from pyproject.toml. The 8 pure moves were already clean and were left alone |
Nothing outside the PR's own file set was touched, and the formatter changed no behaviour: the geometry numbers below are identical before and after it ran.
What passed
| Check | Result |
|---|---|
| DCO | ✅ all four commits signed off |
| File sizes | ✅ largest addition is 22 KB, no heavy arrays |
| Secrets, absolute paths | ✅ clean |
| Compiles | ✅ every changed file passes py_compile |
| Drift metric definition | ✅ unambiguous: mean over the upper triangle of the absolute change in Euclidean pairwise distance |
| Additive to existing geometry | ✅ tetrahedron_10 untouched, and for K = 2..9 the new dispatcher returns bit-identical points (max per-point difference 0.000e+00 lambda) |
Blocking
B1. The sandbox is invisible to the launcher, and eight configurations left the menu
_discover_xperiments (_launcher.py:61) is a non-recursive parameters_dir.glob("*.py"), and load_xperiment (_launcher.py:78) builds a flat dotted path. Counting menu entries by the launcher's own rule (a top-level .py under xparameters/ containing XPARAMETERS):
menu entries on main : 19
menu entries on this PR : 11
The eight that disappeared are the ones moved into the sandbox, all of which define XPARAMETERS: electron_k09_vmode10_ewt_geometry, electron_k09_vmode10_gaussian_saturation, electron_k10_vmode10_1_3_6, electron_k10_vmode10_bcc_lattice, electron_k10_vmode10_gaussian_saturation, electron_k10_vmode10_golden_angle, electron_k11_vmode10_gaussian_saturation, test_live_monitor. The 22 new sandbox configurations never appear either, which means the new locked geometry has no launchable consumer at all.
The note about the sandbox being outside the CI grid does not cover this, since the repository runs no CI; the launcher menu is the only route these files had.
What fixes it: decide whether the sandbox should be launchable, then make the folder and the launcher agree. If it should, recursion plus a dotted relative name works with the existing load_xperiment, keeping the XPARAMETERS test and skipping utils/:
xperiment_files = [
str(p.relative_to(parameters_dir).with_suffix("")).replace("/", ".")
for p in parameters_dir.rglob("*.py")
if p.name != "__init__.py"
and "utils" not in p.parts
and "XPARAMETERS" in p.read_text(encoding="utf-8", errors="ignore")
]If it should not be launchable, then say so in a short README in the folder and note how these configurations are meant to be run, because right now there is no way to run them.
B2. The drift metric raises on the first frame a wave centre deactivates
log_stability_metrics builds positions from active, non-NaN centres only, captures _pairwise_ref on the first call (instrumentation.py:182), and then subtracts matrices of whatever shape the current frame produced (instrumentation.py:186). Reproduced with a stub, no engine and no Taichi:
import types
from openwave.xperiments.m4_ewt.utils import instrumentation as inst
inst.json_logger = types.SimpleNamespace(log_timestep=lambda *a, **k: None)
class WC:
def __init__(self, drop=0):
self.num_sources = 10
self.active = [1] * 10
for i in range(drop):
self.active[9 - i] = 0
self.position_float = [[i * 0.01, 0.0, 0.0] for i in range(10)]
inst.log_stability_metrics(1, WC()) # (0.0, 10)
inst.log_stability_metrics(2, WC(drop=1)) # raisesValueError: operands could not be broadcast together with shapes (9,9) (10,10)
This is a live path, not a hypothetical: force_motion.py:625 sets wave_center.active[wc] = 0 when two centres merge, and the call site in compute_wave_oscillation (_launcher.py:552) is not inside a try, so the run ends there. The metric reports active_wc beside the drift, so wave centres being lost is precisely the case it was written to observe, and it is the case where a K-selectivity run gets interesting.
There is a second defect underneath the first: positions is compacted, so index i stops referring to the same centre after any deactivation. Even when two frames happen to have equal shapes, the comparison would silently pair different centres.
What fixes both: key the reference by wave-centre index rather than by position in a compacted list, and average over the pairs present in both frames.
_pairwise_ref = {} # (i, j) -> reference distance, i and j are wave_center indices
# carry the source index alongside each kept position, then
drifts = [abs(dist[(i, j)] - _pairwise_ref[(i, j)]) for (i, j) in dist if (i, j) in _pairwise_ref]
mean_drift = float(np.mean(drifts)) if drifts else NoneB3. The tetrahedron_10_locked docstring does not describe what the function computes
The docstring states that all separations land on the n·lambda wells and that the outer six sit at 2·lambda. Measured directly from the shipped function, all 45 pair separations, a pair counted as on a well when it is within 0.05 lambda of a positive integer:
import itertools, math
from openwave.xperiments.m4_ewt.xparameters.utils import geometry as g
from openwave.common import constants
for E in (1e-12, 2e-15, 4e-15):
lam = constants.EWAVE_LENGTH / E
p = g.tetrahedron_10_locked(E)
d = [math.dist(a, b) / lam for a, b in itertools.combinations(p, 2)]
on = [x for x in d if abs(x - round(x)) <= 0.05 and round(x) >= 1]
print(len(on), "/", len(d), f"{min(d):.3f}-{max(d):.3f}", f"outer {math.dist(p[0], p[4]) / lam:.4f}")3 / 45 1.000-4.761 lambda outer shell 2.5820 lambda
Identical at all three universe edges, so this is a property of the construction and not of a particular run. Three pairs are on a well: the centre-to-inner distances, exactly 1.0000 lambda, which is the part the docstring gets right. The outer shell is at 2.5820 lambda rather than 2, because r2 = 2·lambda is the radius in the XY plane and then h = r2·sqrt(2/3) = 1.633·lambda is added along z, so the distance from the centre is sqrt(2^2 + 1.633^2).
Worth stating clearly, because the diff reads worse than the change is: this is still a real improvement. The same measurement on the existing tetrahedron_10 gives 0 of 45 at every universe edge, and its scale drifts with the universe (1.401 to 6.672 lambda at 2e-15, 2.803 to 13.345 lambda at 4e-15), which reproduces the measurement already recorded in that function's own docstring. The new one is scale-invariant and puts the inner shell exactly on the first well.
What fixes it for merge: make the docstring state what the code does, which is that the inner three sit at 1·lambda and three of 45 separations are on a well. The separate question of which radii the electron should actually use is routed below and does not need to be settled first.
B4. test_logging_1_3_6.py imports a module that does not exist
from openwave.xperiments.m4_ewt.xparameters.geometry import tetrahedron_10 # line 7That module moved to xparameters/utils/geometry.py in #340, so this file cannot run. It also errors pytest collection at the repository root, though main already carries five such errors, so this adds a sixth rather than breaking something that worked. Repointing the import is the fix.
Requested, not blocking
| # | Item |
|---|---|
| R1 | The description says plots/wc_active.png is produced, but plot_wc_active() is commented out in generate_plots() (plotting.py:293), so it never runs. It also mentions a bundled plot_stability_metrics(), which does not exist anywhere in the repository |
| R2 | electron_k10_vmode10_soft_pressure_0001_2.py carries an X_NAME identical to its sibling, so both would show the same menu label, while the parameters differ (WC_INTERACT_MODE 1 against 3, SEED_BOOST 0.001 against 0.01, and no VELOCITY_DAMPING or CFL_SAFETY). The name also says "soft", which is mode 3. Either the label or the parameters is stale, and only you can say which |
| R3 | generate_positions_by_EWT_geometry_locked duplicates roughly 100 lines of generate_positions_by_EWT_geometry for K = 2..9, verified to return identical points. Delegating for every K except 10 keeps the two from drifting apart later |
| R4 | _launcher.py changes the first instrumentation record from state.frame == 10 to state.frame == 1, which affects every M4 experiment, not only the sandbox. Intentional? |
| R5 | formation10e_tmp.py ships with a _tmp name and defines XPARAMETERS, so it is meant to be selectable. Its docstring also repeats that only K = 10 spans the lock-in wells, which the B3 measurement contradicts |
Questions
electron_k10_vmode10_soft_pressure_0001_2.pyis the only consumer of the new locked geometry, and it currently cannot be launched. Is a result meant to come from it in this PR, or is the generator landing ahead of the runs?- The drift is reported in
[vox]on the plot axis. Confirming that matches the units ofposition_floatwould make the plot readable without reading the code.
Model author
Tagging @jeffsyee on exactly one point, and nothing else in this review: what radii should the 1-3-6 electron use? The existing tetrahedron_10 docstring records, in a maintainer commit, that its radii are normalized constants that do not scale with the universe, that none of its 45 separations land on a well, and that the choice of radii is the author's call. This PR is the first change to act on that, and B3 is where the answer lands. Scaling the outer shell by 0.7746 would put it at a true spherical 2·lambda, but that is a physics decision rather than a documentation fix, so it is not treated as blocking here. Everything else above is mechanical or contributor-owned and does not need the author.
What was not checked
No simulation was run, so nothing here speaks to whether the locked geometry is actually more stable, only to what the generator computes. The drift plot's [vox] units were not verified against the engine. The physics parameters of the 22 new configurations were not audited beyond the two near-duplicates in R2.
|
I'm going away for an extended long weekend. I'll return to this topic next week. Best regards. |
|
No rush at all, this will keep until you are back. One suggestion on method while it sits, nothing about the code itself. Looking at #340 and this one together, the K-selectivity study is running through the rendered path: new potential modes in The platform separates two solvers on purpose. The headless one, plain scripts under a model's M7 states the ordering as "headless first, rendering graduates once the electron is canonical, no GUI work before the physics is canonical", and M8 makes its rendering port a task explicitly gated on validated dynamics. M4 now has the full scaffolding created, same as M5, M7 and M8: https://github.com/openwave-labs/openwave/tree/main/openwave/xperiments/m4_ewt/research That gives you Concretely, why this would help the sweep you are running. It is roughly K in {9,10,11} by V_MODE by interaction mode by boost/pressure by seed geometry. As launcher configs that is around 30 windows someone has to sit and watch, and the conclusion ends up living with whoever watched them. As one runner script in The drift metric you added is the right observable for exactly this. It just wants to be a number returned into a results table, not only a line on a live panel. So the shape I would suggest: The sweep as a runner script under Worth saying explicitly: a documented negative counts fully here. "K is not selected by this potential, here is the script, here is the evidence" is a real result, and it closes an open question that is sitting in the M4 briefing right now. Thanks for the work on this. The stabilisation engine from #340 is a solid contribution, and K-selectivity is the headline open problem on M4, so it is good to have someone on it. |
Summary
This PR introduces a Wave‑Center stability monitoring subsystem that tracks the structural integrity of any WC configuration during a simulation, moves exploratory K‑selectivity experiments into a dedicated
sandbox_k_selectivity/folder, and adds a new lock‑in‑scaled 1‑3‑6 geometry generator with proper rotation support.Changes
1. WC stability metrics (
utils/)instrumentation.py– addedlog_stability_metrics(). Computes mean pairwise distance drift (relative to the first sampled frame) and active WC count. Data is appended to the existing JSON log.sampling.py– addedsample_stability_metrics()and new circular buffers (_stability_timesteps,_stability_drifts,_stability_active). The live‑monitor JSON file now carries the extra keysstability_timesteps,mean_drifts,active_wcs.plotting.py– addedplot_wc_drift()andplot_wc_active()for post‑simulation plots._read_timestep_data()now skips log records that lackdisplacement_am(stability records are mixed into the same JSON array).live_monitor_viewer.py– extended to a 3‑panel layout. Panel 3 shows the mean pairwise drift in real time._launcher.py– wired the new metrics into the main loop. Stability data is sampled every 60 frames (together with the existing timestep log) and the monitor file is flushed immediately so the live viewer updates without delay.2. Sandbox directory (
xparameters/sandbox_k_selectivity/)electron_k09_vmode10_ewt_geometry.pyelectron_k09_vmode10_gaussian_saturation.pyelectron_k10_vmode10_1_3_6.pyelectron_k10_vmode10_bcc_lattice.pyelectron_k10_vmode10_gaussian_saturation.pyelectron_k10_vmode10_golden_angle.pyelectron_k11_vmode10_gaussian_saturation.pytest_live_monitor.py3. Locked geometry generator (
utils/geometry.py)tetrahedron_10_locked()– new 1‑3‑6 geometry where inner and outer radii are multiples ofLOCK_SPACING(1 λ and 2 λ). All pair separations land on the standing‑wave lock‑in wells. Supports rotation (Euler angles Z‑Y‑X) and perturbation.generate_positions_by_EWT_geometry_locked()– dispatcher that usestetrahedron_10_lockedfor K = 10 and the same explicit polyhedral geometries asgenerate_positions_by_EWT_geometryfor K = 2…9.How to use the stability metrics
Any experiment with
"INSTRUMENTATION": truenow automatically records WC drift and active count.plots/wc_drift.pngandplots/wc_active.png(or the combinedstability_metrics.pngif you use the bundledplot_stability_metrics()) give the full time history.Notes for reviewers
wave_center.position_floatandwave_center.active.sandbox_k_selectivity/folder is intentionally not part of the CI test grid for now – it’s a workspace for ongoing K‑selectivity studies.tetrahedron_10_lockedis provided alongside the existingtetrahedron_10so that existing experiments are not affected.test