Skip to content

refactor(calibrate): name the motor-position numbers - #88

Open
cn0303 wants to merge 1 commit into
huggingface:mainfrom
cn0303:refactor/calibrate-position-constants
Open

refactor(calibrate): name the motor-position numbers#88
cn0303 wants to merge 1 commit into
huggingface:mainfrom
cn0303:refactor/calibrate-position-constants

Conversation

@cn0303

@cn0303 cn0303 commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

calibrate.py decided "is this a plausible motor-position reading" with bare
literals repeated in three places, plus two more single-use thresholds:

  • pos <= 0 or pos >= 5000 (skip invalid) in get_status
  • pos > 0 and pos < 5000 (keep valid) in _step_range_recording, twice
  • abs(pos - prev) > 2000 : the encoder wrap-around jump
  • range_diff < 100 : the "joint barely moved" warning threshold

Same concepts written slightly differently, so changing the valid range meant
finding every copy and hoping they agreed.

The change

Hoist the values into named module constants and route the three plausible-range
checks through one helper:

_MIN_VALID_POSITION = 0
_MAX_VALID_POSITION = 5000
_MAX_POSITION_JUMP = 2000
_MIN_CALIBRATION_RANGE = 100


def _is_valid_position(pos: float) -> bool:
    return _MIN_VALID_POSITION < pos < _MAX_VALID_POSITION

The three filter sites become _is_valid_position(pos) / not _is_valid_position(pos), and the two remaining thresholds read
_MAX_POSITION_JUMP / _MIN_CALIBRATION_RANGE.

No behaviour change

This is a pure refactor. The helper's bounds are exclusive on both ends, which
matches the old pos > 0 and pos < 5000 exactly (and its negation pos <= 0 or pos >= 5000). The wrap-around and minimum-range thresholds keep their exact
values.

Testing

Behaviour-preserving, so the proof is the existing calibrate suite staying green
plus a new parametrised boundary test on _is_valid_position pinning the
exclusive bounds (0 and 5000 both invalid; 1, 4095, 4999 valid). pytest tests/test_calibrate.py: 15 passed. Full suite: 207 passed (the only 3 failures
are pre-existing on main, in dataset_repair/jobs, unrelated). ruff check and ruff
format green; pre-commit (mypy, bandit, typos) green.

There is no hardware before/after to run here; the values and comparisons are
unchanged, only their names.

The plausible-encoder-range check was inlined as `pos > 0 and pos < 5000` (and
its negation) in three places, with the wrap-around jump threshold (2000) and
the minimum-sweep threshold (100) as bare literals elsewhere. Same concepts,
slightly different spellings, easy to drift when one changes.

Hoist them into named module constants and an `_is_valid_position()` helper, and
call it from the three filter sites. Pure refactor, no behaviour change: the
helper's bounds are exclusive on both ends, matching the old inline checks. A
parametrised boundary test pins that behaviour.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant