fix(checks): fail closed on unmeasurable joint-motion steps (#546) - #549
Conversation
_joint_motion_profile yields NaN velocity where a step duplicates its timestamp or a position is NaN. Both consumers compared that NaN with a plain operator -- NaN comparisons are False -- so unmeasurable steps were counted as compliant and not-idle while their duration stayed in the percentage denominators: a stream whose every step duplicates its timestamp reported violation_pct 0.0 (the duplicate stamp hiding the jump), an all-NaN channel reported idle_fraction 0.0, and joint discontinuity's max_abs_velocity computed np.nanmax over nothing and refused at the catalog append instead of inside the check (Hebbian-Robotics#546). The profile now carries an explicit measurable mask -- strictly positive duration AND finite positions on every joint -- and both consumers compute over measurable steps only: violation_pct divides by the measurable count, idle_fraction excludes unmeasurable time from the denominator, max_abs_velocity is a finite np.max. A stream with nothing measurable reports its counts (velocity_sample_count, velocity_measurable_step_count, nonpositive_dt_count; the idle-side prefixed equivalents) and withholds the percentage keys instead of emitting 0.0 or NaN. Counts are per-check prefixed per the module's no-shared-keys rule. Tests: duplicate-stamp refusal (red) versus advancing-stamp 100.0 (green) on identical position hops; all-NaN abstention from both checks; a denominator-dilution stream where the honest percentage is 1/3, not 1/5; and a finite-value sweep over every measurement these paths emit. Mutation-verified: dropping the mask from the percentage denominator turns the dilution test red; reverting the idle abstain turns three of the four red.
kstonekuan
left a comment
kstonekuan
left a comment
There was a problem hiding this comment.
The fix is right and each half of measurable is held: dropping deltas_s > 0 or finite_jump fails different cases.
One thing to fix. np.nanmax at checks.py:124 warns on an all-NaN row, so the suite gains four RuntimeWarning: All-NaN slice encountered that main does not have, and a user processing a duplicate-stamped stream gets numpy noise in exactly the case this PR exists to handle.
You already compute the mask that says which rows those are.
np.nanmax on every row warns once per all-NaN slice, so a duplicate-stamped or NaN-position stream -- exactly what Hebbian-Robotics#546 is about -- got RuntimeWarning noise from a check whose job is to measure it quietly. The measurable mask already names the all-NaN rows: fill the max from those rows with np.max, leave the rest NaN, and np.nanmax is gone (Hebbian-Robotics#549 review).
|
Good catch. Replaced Verified the suite with Pushed 9c38a1d. |
kstonekuan
left a comment
kstonekuan
left a comment
There was a problem hiding this comment.
The masked assignment is the right shape, and the comment explaining why it replaced np.nanmax will save the next person rediscovering it.
Verified against main merged in: -W error::RuntimeWarning runs clean at 2166 passed, so the four warnings are gone rather than filtered. All six guards hold under mutation. Dropping finite_jump reddens 2, dropping deltas_s > 0 reddens 3, widening either percentage denominator back to all steps reddens 1 each, and removing either nothing-measurable early return reddens 2 and 1.
Taking the advanced ones is exactly where the leverage is. Thanks.
2 participants
Fixes #546
Summary
As detailed in the issue,
_joint_motion_profilewas treating NaN velocity steps (from duplicate timestamps or NaN positions) as compliant becauseNaN > limitandNaN < epsilonboth evaluate toFalse. This PR carries an explicitmeasurablemask through the profile and computes all compliance metrics over measurable data only, ensuring unmeasurable steps are excluded from percentages and dead/NaN streams abstain rather than emitting pass-baiting numbers.Changes
src/hflow/checks.py:measurablemask to_JointMotionProfile(strictly positive dt AND finite position jumps).joint_discontinuity:violation_pctdivides by measurable count only;violation_countonly counts measurable steps;max_abs_velocityis a finitenp.max(NaN refused inside the check).idle_fraction: Unmeasurable steps are excluded from both numerator and denominator. Removed the deadif total_span_s else 0.0fail-open.*_sample_count/*_measurable_step_count/nonpositive_dt_countand withhold percentage keys.tests/test_checks_nan_failopen.pywith 5 core proofs:violation_pct=100.0.idle_fractionabstains instead of storing0.0.nonpositive_dt_countis visible wherever a percentage is emitted.Mutation Proof
1 failed, 69 passed).0.0and fails (3 failed, 1 passed).Gates
ruff check,ruff format,ty(repo-wide): Clean.