Skip to content

Add golden regression test for motor2 computation - #36

Merged
sadda merged 8 commits into
mainfrom
20-add-unit-tests
Sep 14, 2026
Merged

sadda merged 8 commits into
mainfrom
20-add-unit-tests

Conversation

@haitao-ZCU

Copy link
Copy Markdown
Collaborator
  • Add a regression test for motor2 (tests/test_motor2_golden.py), using the baseline from commit c438e18.
  • Provenance and how to reproduce it are in tests/golden/motor2_baseline/readme.md.
  • The baseline is precomputed once, not rerun on every test, and stored as CSV instead of pickle format.

@haitao-ZCU haitao-ZCU linked an issue Sep 9, 2026 that may be closed by this pull request
@sadda

sadda commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

@haitao-ZCU This PR is very strange.

  • It uses some commit, which does not even lie on main.
  • It has completely different structure than the old tests.
  • It has the old names.
  • It does not precompute the Ansys values for the first motor.
  • The relative tolerance of 10% seems to be too high.

@haitao-ZCU

Copy link
Copy Markdown
Collaborator Author

@sadda

Thanks for pointing this out. You are right about point 1. c438e18 only exists on origin/vs_5f and not on main. I will regenerate the baseline from a recent commit on main that is known to be bug free.

For point 4, my understanding is that you mean motor1's existing test (test_ansys_regression.py) also does not use a precomputed baseline, and that motor1 should have a similar precomputed baseline test for consistency with motor2. So, we should use precomputed baseline for motor1 and motor2 respectively?

@sadda

sadda commented Sep 9, 2026 •

Copy link
Copy Markdown
Collaborator

@haitao-ZCU About 4, yes. Also tests for both motors should be as unified as possible.

@haitao-ZCU

Copy link
Copy Markdown
Collaborator Author

@sadda

I have regenerated both baselines from the current main branch, not from a commit off a side branch like before, and pushed this to the PR.

Currently, motor1 and motor2 golden tests share the same structure: one Design instance, one candidate rotor, compared against a precomputed CSV baseline.

On "different structure than old tests", I think test_ansys_regression.py does two things ours doesn't.

  • It compares a live frozen legacy run against a live current run instead of a precomputed baseline.
  • It reuses one Design across two candidate rotors (seeds 42 and 43). I tried that too, but reusing a Design for a second rotor adds noticeably more solver error than a single fresh solve, likely from mesh or solution state carried over between solves. A fixed baseline is fragile to that, so I kept one candidate per test for now.

On tolerance,
Both motors are nearly deterministic on the same machine across 30 repeated runs. There is a small but stable offset when comparing the baseline against runs on a different machine, about 0.55% for motor1 and 2.14% for motor2, so I set tolerance of rtol=0.01 for motor1 and rtol=0.03 for motor2.

The raw data from the 30 runs is attached. Column C (max_rel_err_pct) is the max relative error percentage for that run against the baseline.

motor_errors_results_default_nper.csv

@sadda

sadda commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator

@haitao-ZCU If there are such big differences across multiple machines, I think that we need to go back to the old tests (test_ansys_regression) and generalise them for the second motor. This will mean twice the runtime, but it is still better than setting arbitrary thresholds for relative errors.

If there are any problems with the second motor, we need to fix the code and not the tests. If the tests work for one seed, but not for both, we need to find what went wrong, it is possible that some artifacts stayed there and were not deleted (meshing?).

Are we currently convinced that the computation for the second motor works fine? If not, there is no point, to use the current implementation as a baseline.

@haitao-ZCU

Copy link
Copy Markdown
Collaborator Author

@sadda

On your second paragraph (point 2):
I found and fixed a real bug:
The rotor mesh operation was never deleted along with the rotor geometry, this causes a name collision on the next candidate rather than cleanly reused. This looks like the actual cause of the seed 42 vs seed 43 mismatch, because that works after applying the fix.
About this fix, I have proposed a separate PR, validated by reusing one Design across 10 repeated candidates for both motors, errors are very small (about 2.72e-6 for motor1, and 1.15e-8 for motor2).

As to this PR (golden baseline), I'm still working on it.

@haitao-ZCU haitao-ZCU closed this Sep 14, 2026
@sadda sadda reopened this Sep 14, 2026
@sadda

sadda commented Sep 14, 2026

Copy link
Copy Markdown
Collaborator

I am reopening it because #38 addressed a different issue.

@haitao-ZCU

Copy link
Copy Markdown
Collaborator Author

Extend the legacy vs live regression test structure to motor2 (tests/test_ansys_regression_synrm_5f_40s.py), mirroring the existing test_ansys_regression.py for motor1. The legacy reference is an old pre-refactor implementation of motor2 (tests/legacy/design2.py).

Currently, the tolerance for motor2 is rtol=1e-3 (relative tolerance), atol=2e-4 (absolute tolerance), tested across several different setpoints on both Windows and Linux (server).

Testing setpoints:
(7.0711, 7.0711, 0.0, 0.0); (0.0, 0.0, 7.0711, 7.0711); (7.0711, 0.0, 0.0, 0.0); (0.0, 7.0711, 0.0, 0.0); (0.0, 0.0, 7.0711, 0.0); (0.0, 0.0, 0.0, 7.0711)

@sadda
sadda merged commit e447df8 into main Sep 14, 2026
6 checks passed
@haitao-ZCU
haitao-ZCU deleted the 20-add-unit-tests branch September 16, 2026 13:37
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.

Add unit tests

2 participants