Skip to content

motors: name anchors after the machine, not their position in the file - #27

Merged
sadda merged 1 commit into
mainfrom
26-rename-anchors
Sep 7, 2026
Merged

sadda merged 1 commit into
mainfrom
26-rename-anchors

Conversation

@smidl

@smidl smidl commented Sep 3, 2026 •

Copy link
Copy Markdown
Contributor

Closes #26.

The problem

Geometry2/Computation2 are named for being the second machine defined, not for the machine they describe. CONTRIBUTING.md uses the word "anchor" three times and never defines it — the defining section survives on the contributing branch and was dropped in #23, leaving line 63 reading "add an motors" mid-substitution. Detail in #26.

What changed

Pure rename plus docs — no behaviour change.

before after
motors/motor1.py → Geometry, Computation motors/synrm_3f_36s.py → Geometry, Computation
motors/motor2.py → Geometry2, Computation2 motors/synrm_5f_40s.py → Geometry, Computation

One module per anchor, named <class>_<phases>f_<slots>s. Each exports plain Geometry/Computation, so the module name carries the identity and the class names stay uniform:

from motors.synrm_5f_40s import Computation, Geometry

The numbering existed only because both machines shared one namespace. Per-anchor modules remove the need for it, rather than trading Geometry2 for a longer awkward class name.

Turns are deliberately not in the name. Nc is routinely swept, so a name containing it goes stale; the name carries only what the anchor fixes for good — class, phases, slots. Same reasoning for currents and bus voltage.

Old paths kept as deprecation shims. motors/motor1.py and motors/motor2.py re-export the old names, so branches predating this keep importing and migrate on their own schedule. Verified: motors.motor2.Geometry2 is motors.synrm_5f_40s.Geometry → True.

Also: each anchor module gets a one-line docstring naming its machine, the two test modules follow the anchor names, and CONTRIBUTING regains the anchor definition with the motors/ layout row and the "add an motors" sentence fixed.

Verification

Both anchor modules were diffed against the motor1.py/motor2.py they replace, with the class renames normalised away: identical apart from the rename and the docstring.

  • pytest → 26 passed, 1 deselected — same as main
  • ruff check . / ruff format . --check → clean

One thing reviewers should know about renames

#28 and #29 both landed in motor2.py while this branch was open, and this PR produced no merge conflict for either — because moving a file makes the new path an unrelated add, so git replays the branch's copy verbatim and silently drops what landed meanwhile. Both fixes (np.array unit conversion + its import numpy as np, and the set_derived_params stub removal) had to be carried across by hand and re-verified by diff.

That is an argument for landing a rename promptly rather than letting it age: every subsequent fix to the moved file has to be hand-carried, with nothing to warn you.

@sadda

sadda commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Please use git mv instead of a hard-copy so that I can see the actual changes.

@smidl
smidl force-pushed the 26-rename-anchors branch 2 times, most recently from fc0d469 to 745560c Compare September 4, 2026 10:26
@smidl
smidl marked this pull request as ready for review September 4, 2026 10:26
`Geometry2`/`Computation2` are named for being the second machine defined, not for
the machine they describe. Nothing in the name says 5-phase or 40-slot, and nothing
distinguishes it from the other 5-phase variants people run on branches.

One module per anchor, named `<class>_<phases>f_<slots>s`:

  motors/motor1.py -> motors/synrm_3f_36s.py
  motors/motor2.py -> motors/synrm_5f_40s.py

Each exports plain `Geometry` and `Computation`, so the module name carries the
identity and the class names stay uniform. The numbering existed only because both
machines shared a namespace; per-anchor modules remove the need for it.

  from motors.synrm_5f_40s import Computation, Geometry

The name carries only what the anchor fixes for good -- class, phases, slots. Turns
are deliberately left out: `Nc` is routinely swept, so a name containing it goes
stale. Same reasoning for currents and bus voltage.

Also restores the anchor definition to CONTRIBUTING (it survives on the
`contributing` branch but was dropped in #23, leaving "anchor" used three times and
never defined, and the sentence "add an **motors**" mid-substitution), and fixes that
sentence.

No behaviour change: the two anchor modules were diffed against the `motor1.py` /
`motor2.py` they replace, with the class renames normalised away, and are identical
apart from a one-line docstring -- including the fixes from #28 and #29, which landed
in `motor2.py` while this branch was open and had to be carried across by hand.

Callers must update their import path; there is no compatibility shim, so a stale
import fails loudly rather than silently resolving to an aliased class.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@smidl

smidl commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

Good catch — I did use git mv, but the deprecation shims I left at motors/motor1.py and motors/motor2.py meant both the old and new paths existed at the branch tip, so git paired base↔head on the old path and reported the anchor module as a new file. It rendered exactly like a hard copy. Splitting into commits wouldn't have helped, since the PR view diffs base against head.

Dropped the shims and force-pushed. It now reports as renames:

 CONTRIBUTING.md                                 | 22 +++++++++++++++++++---
 motors/{motor1.py => synrm_3f_36s.py}           |  2 ++
 motors/{motor2.py => synrm_5f_40s.py}           | 12 +++++++++---
 notebooks/run.py                                |  2 +-
 notebooks/run_optimization.py                   |  2 +-
 tests/test_ansys_regression.py                  |  2 +-
 tests/{test_design.py => test_synrm_3f_36s.py}  |  2 +-
 tests/{test_design2.py => test_synrm_5f_40s.py} | 10 +++++-----
 8 files changed, 39 insertions(+), 15 deletions(-)

39/15 instead of the previous 741/699. The only content changes are a one-line docstring per anchor module, the class renames in the 5-phase one, and import paths.

I also reverted the test-fixture renames I'd made (geometry2 → geometry etc.). They were churn that dropped the test file below git's rename-similarity threshold without serving the point of the PR; the fixture bodies change, the names don't. Happy to tidy those separately if you want them consistent.

Verified independently of the diff: both anchor modules were compared against the motor1.py/motor2.py they replace with the class renames normalised away — identical apart from the docstring, including the #28 and #29 fixes that landed while this branch was open. pytest 26 passed / 1 deselected, same as main; ruff clean.

One consequence of dropping the shims: callers must update the import path, and there is no alias, so a stale import fails loudly instead of silently resolving. That affects pl_mag2.0, the newmodel_haitao branches and my own vs_5f — a one-line change each. I think loud is the right trade here, but say the word if you'd rather keep shims for a release and accept the noisier diff.

@smidl
smidl force-pushed the 26-rename-anchors branch from 745560c to a27793b Compare September 6, 2026 10:17
@sadda
sadda merged commit 51f519d into main Sep 7, 2026
6 checks passed
@smidl
smidl deleted the 26-rename-anchors branch September 8, 2026 17:33
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.

Anchors should be named after the machine, not numbered (Geometry2)

2 participants