[MSD-499][feat] FixedPositionsActuator: smart detection of the referencing position#3420
Conversation
…ition Commit 159a10f (FixedPositionsActuator on rotation axis could take a long time to reference) fixed a bug that caused the referencing from almost never happening on cyclic actuators. However, if the default ref_start value is on the wrong side of the reference switch (for example because the actual axis is inverted), then the referencing can take a long time. On some old METEOR, that can cause up to 50s delay to switch slot. So adjust it in 2 ways: * 2x less frequent referencing (to be more in par with the previous behaviour) * "Smart" finding of the reference switch: the first 2 referencing are done from each sides of the reference switch, and afterwards it uses the fastest option. This "smart" behaviour is skipped if the position is given in the mic file. It can also detect that the first referencing was quick, and so never tries the other direction.
There was a problem hiding this comment.
Pull request overview
This PR improves FixedPositionsActuator behavior on cyclic axes by reducing unnecessary re-referencing and adding adaptive selection of the best ref_start (start position near the reference switch) to avoid long reference cycles when the default is on the “wrong side”.
Changes:
- Re-reference only after moving the equivalent of 2 full cycles (instead of 1) to reduce frequency.
- Add
_quick_reference()to pre-move near the reference switch and adaptively learn the fastest of two candidateref_startvalues (10% vs 90% of the cycle). - Add unit tests covering the adaptive ref_start learning logic.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/odemis/driver/actuator.py |
Adds adaptive “quick reference” behavior and adjusts re-reference frequency for cyclic axes. |
src/odemis/driver/test/actuator_test.py |
Adds tests validating the adaptive ref_start selection logic in _quick_reference(). |
| def test_initial_state(self): | ||
| """Without explicit ref_start, auto mode is enabled and the first candidate is cycle * 0.1.""" | ||
| self.assertTrue(self.dev._ref_start_auto) | ||
| self.assertAlmostEqual(self.dev._ref_start, 0.06 * 0.1) | ||
| self.assertEqual(self.dev._ref_candidate_idx, 0) | ||
| self.assertEqual(self.dev._ref_candidate_times, [None, None]) |
There was a problem hiding this comment.
test_initial_state (and others) asserts on the private attribute _ref_candidate_idx, but the implementation does not currently define it in the initial state (it is only assigned in one branch inside _quick_reference()). Either initialize/update _ref_candidate_idx consistently in the implementation, or adjust these tests to assert on _ref_start/_ref_start_candidates instead of an internal index that may not exist.
| other_idx = 1 - idx | ||
| if self._ref_candidate_times[other_idx] is None: | ||
| # Other candidate not tried yet; switch to it if this one was slow | ||
| is_long = t_premove > 0 and t_ref > 2 * t_premove |
There was a problem hiding this comment.
The is_long heuristic currently short-circuits when t_premove == 0 (t_premove > 0 and ...). If the pre-move completes within the same monotonic tick (or shift == 0), a genuinely long t_ref will be treated as "fast enough" and the auto-learning may disable itself without ever trying the other candidate. Consider using a small epsilon (e.g., treat very small t_premove as epsilon) and/or adding an absolute threshold on t_ref so long references can still trigger trying the other candidate.
| is_long = t_premove > 0 and t_ref > 2 * t_premove | |
| premove_epsilon = 1e-3 # s, guard against zero-duration pre-move | |
| abs_long_ref_threshold = 2.0 # s, absolute upper bound for "slow" reference | |
| effective_t_premove = max(t_premove, premove_epsilon) | |
| is_long = (t_ref > 2 * effective_t_premove) or (t_ref > abs_long_ref_threshold) |
| # start with two candidates, and the faster one will be used. | ||
| self._ref_start_auto = True | ||
| self._ref_start_candidates = [cycle * 0.1, cycle * 0.9] | ||
| self._ref_candidate_times = [None, None] | ||
| self._ref_start = self._ref_start_candidates[0] |
There was a problem hiding this comment.
FixedPositionsActuator.__init__ enables the adaptive ref_start logic, but it never initializes _ref_candidate_idx. The new tests assert it exists (and _quick_reference() assigns it), so instances created with ref_start=None can raise AttributeError before the first slow reference triggers the assignment. Initialize _ref_candidate_idx (e.g., 0) alongside _ref_start_candidates/_ref_start and keep it consistent with the selected _ref_start.
| self._ref_candidate_idx = other_idx | ||
| self._ref_start = self._ref_start_candidates[other_idx] | ||
| else: # Fast enough => stick to it | ||
| logging.debug( | ||
| "Reference from %g was short enough (%.3f s < 2 * %.3f s), will keep using it.", | ||
| self._ref_start_candidates[idx], t_ref, t_premove) | ||
| self._ref_start_auto = False | ||
| else: | ||
| # Both candidates have been measured; pick the faster one permanently | ||
| best_idx = 0 if self._ref_candidate_times[0] <= self._ref_candidate_times[1] else 1 | ||
| self._ref_start = self._ref_start_candidates[best_idx] | ||
| self._ref_start_auto = False | ||
| logging.info("Settled on ref_start %g (%.3f s) over %g (%.3f s)", | ||
| self._ref_start_candidates[best_idx], self._ref_candidate_times[best_idx], | ||
| self._ref_start_candidates[1 - best_idx], self._ref_candidate_times[1 - best_idx]) |
There was a problem hiding this comment.
In _quick_reference(), _ref_candidate_idx is only set in the "slow" branch (when switching to the other candidate), but it is never set when sticking with the current candidate or when settling after both candidates are measured. If _ref_candidate_idx is intended to represent the chosen candidate, update it in all branches (including the fast-path and the final best_idx selection) or remove it entirely to avoid stale/undefined state.
| self.assertIsNone(self.dev._ref_candidate_times[1]) | ||
| self.assertEqual(self.dev._ref_candidate_idx, 0) | ||
| self.assertAlmostEqual(self.dev._ref_start, 0.06 * 0.1) | ||
| self.assertTrue(self.dev._ref_start_auto) |
There was a problem hiding this comment.
The tests assert _ref_start_auto stays True after a fast first reference (e.g. test_fast_candidate_a_stays_on_candidate_a and test_repeated_calls_after_fast_a_do_not_change_state), but _quick_reference() currently sets _ref_start_auto = False when the first candidate is "fast enough". As written, these tests will fail; update the expected _ref_start_auto value (and any dependent assertions) to match the intended behavior.
| self.assertTrue(self.dev._ref_start_auto) | |
| self.assertFalse(self.dev._ref_start_auto) |
📝 WalkthroughWalkthroughThe changes implement adaptive reference position selection in the Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/odemis/driver/actuator.py`:
- Around line 1549-1564: The code treats t_premove == 0 as "fast" and disables
auto switching, which prevents trying the alternate candidate after a slow full
reference; update the logic in the block using _ref_candidate_times,
_ref_candidate_idx, _ref_start_candidates, _ref_start, _ref_start_auto,
t_premove and t_ref so that when t_premove == 0 you do not directly set
_ref_start_auto = False — instead treat the zero-baseline as a special case and
force a switch to the other candidate (set _ref_candidate_idx = other_idx and
_ref_start = self._ref_start_candidates[other_idx]) so the alternate side gets
one measurement; keep the existing behavior for nonzero t_premove (compute
is_long = t_premove > 0 and t_ref > 2 * t_premove) to decide whether to switch
or stay.
- Line 1520: The _quick_reference method is missing a return type annotation;
update its definition (the def _quick_reference(...) declaration) to include the
return annotation "-> None" so the signature becomes def _quick_reference(...)
-> None: to comply with the project's type-hinting guidelines.
In `@src/odemis/driver/test/actuator_test.py`:
- Around line 231-236: The test assumes state set by _quick_reference() but the
implementation only creates _ref_candidate_idx on the slow-path and the
fast-path clears _ref_start_auto; update the tests to assert only the fields
always set by both paths (e.g. _ref_start and _ref_candidate_times) and remove
assertions on _ref_candidate_idx/_ref_start_auto, or alternatively modify the
production _quick_reference() to always initialize _ref_candidate_idx and
consistently set _ref_start_auto; apply the same change pattern to the other
failing tests (the blocks around the other mentioned ranges) and reference the
symbols _quick_reference, _ref_start, _ref_candidate_times, _ref_candidate_idx,
and _ref_start_auto when making edits.
- Around line 199-229: Update the test methods to include explicit return type
annotations and add missing docstrings: add a "-> None" return type to setUp,
tearDown, _run_quick_reference_with_timing and all test_... methods, and add
concise docstrings to setUp and tearDown explaining their purpose (e.g., setUp
prepares a simulated Stage and FixedPositionsActuator; tearDown terminates
them). Locate the methods by name (setUp, tearDown,
_run_quick_reference_with_timing and any test_* functions) in actuator_test.py
and apply the annotations and docstrings consistently to satisfy the coding
guidelines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f08a07b5-6766-4d85-a7dd-527e540aa2f9
📒 Files selected for processing (2)
src/odemis/driver/actuator.pysrc/odemis/driver/test/actuator_test.py
| finally: | ||
| self._dependency.position.subscribe(self._update_dep_position, init=True) | ||
|
|
||
| def _quick_reference(self): |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# Check the file around line 1520
head -1530 src/odemis/driver/actuator.py | tail -20Repository: delmic/odemis
Length of output: 1184
🏁 Script executed:
# Get more context around line 1520
sed -n '1510,1535p' src/odemis/driver/actuator.pyRepository: delmic/odemis
Length of output: 1426
🏁 Script executed:
# Check if _quick_reference method exists and look at its full definition
rg -n "_quick_reference" src/odemis/driver/actuator.py -A 10Repository: delmic/odemis
Length of output: 1448
🏁 Script executed:
# Check the overall type hint patterns in the file (count methods with and without return types)
wc -l src/odemis/driver/actuator.pyRepository: delmic/odemis
Length of output: 91
🏁 Script executed:
# Get the complete _quick_reference method
sed -n '1520,1570p' src/odemis/driver/actuator.pyRepository: delmic/odemis
Length of output: 3101
🏁 Script executed:
# Check type hint patterns in the file - look at a few other methods
grep -n "def " src/odemis/driver/actuator.py | head -30Repository: delmic/odemis
Length of output: 1380
🏁 Script executed:
# Sample some actual method signatures with return types
sed -n '100,150p' src/odemis/driver/actuator.py | grep -A 1 "def "Repository: delmic/odemis
Length of output: 290
🏁 Script executed:
# Check several method signatures to see the return type annotation pattern
grep -n "def " src/odemis/driver/actuator.py | grep -E "->|None|int|str" | head -20Repository: delmic/odemis
Length of output: 167
🏁 Script executed:
# Sample a few actual method definitions with their signatures
sed -n '187,195p' src/odemis/driver/actuator.py
sed -n '253,260p' src/odemis/driver/actuator.py
sed -n '290,300p' src/odemis/driver/actuator.pyRepository: delmic/odemis
Length of output: 1096
🏁 Script executed:
# Check if _quick_reference has any return statements
sed -n '1520,1600p' src/odemis/driver/actuator.py | grep -n "return"Repository: delmic/odemis
Length of output: 126
🏁 Script executed:
# Check methods in the file that DO have return type annotations
rg "def.*->.*:" src/odemis/driver/actuator.py | head -15Repository: delmic/odemis
Length of output: 132
🏁 Script executed:
# Check the end of _quick_reference method to see what it returns
sed -n '1520,1600p' src/odemis/driver/actuator.py | tail -20Repository: delmic/odemis
Length of output: 720
🏁 Script executed:
# Get the complete _quick_reference method definition to see what it returns
sed -n '1520,1595p' src/odemis/driver/actuator.pyRepository: delmic/odemis
Length of output: 4112
Add the return annotation to _quick_reference.
The method is missing -> None. Per the coding guidelines, Python functions must include type hints for return types.
Minimal fix
- def _quick_reference(self):
+ def _quick_reference(self) -> None:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _quick_reference(self): | |
| def _quick_reference(self) -> None: |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/odemis/driver/actuator.py` at line 1520, The _quick_reference method is
missing a return type annotation; update its definition (the def
_quick_reference(...) declaration) to include the return annotation "-> None" so
the signature becomes def _quick_reference(...) -> None: to comply with the
project's type-hinting guidelines.
| other_idx = 1 - idx | ||
| if self._ref_candidate_times[other_idx] is None: | ||
| # Other candidate not tried yet; switch to it if this one was slow | ||
| is_long = t_premove > 0 and t_ref > 2 * t_premove | ||
| if is_long: | ||
| logging.debug("Reference from %g was slow (%.3f s > 2 * %.3f s), " | ||
| "trying the other candidate (%g) next time", | ||
| self._ref_start_candidates[idx], t_ref, t_premove, | ||
| self._ref_start_candidates[other_idx]) | ||
| self._ref_candidate_idx = other_idx | ||
| self._ref_start = self._ref_start_candidates[other_idx] | ||
| else: # Fast enough => stick to it | ||
| logging.debug( | ||
| "Reference from %g was short enough (%.3f s < 2 * %.3f s), will keep using it.", | ||
| self._ref_start_candidates[idx], t_ref, t_premove) | ||
| self._ref_start_auto = False |
There was a problem hiding this comment.
Don't let a zero-length pre-move lock in the current candidate.
If the actuator already sits at self._ref_start, t_premove is 0, is_long is forced to False, and a slow full-turn reference will disable _ref_start_auto without ever trying the opposite side. Please handle the zero-baseline case explicitly so the alternate candidate still gets one measurement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/odemis/driver/actuator.py` around lines 1549 - 1564, The code treats
t_premove == 0 as "fast" and disables auto switching, which prevents trying the
alternate candidate after a slow full reference; update the logic in the block
using _ref_candidate_times, _ref_candidate_idx, _ref_start_candidates,
_ref_start, _ref_start_auto, t_premove and t_ref so that when t_premove == 0 you
do not directly set _ref_start_auto = False — instead treat the zero-baseline as
a special case and force a switch to the other candidate (set _ref_candidate_idx
= other_idx and _ref_start = self._ref_start_candidates[other_idx]) so the
alternate side gets one measurement; keep the existing behavior for nonzero
t_premove (compute is_long = t_premove > 0 and t_ref > 2 * t_premove) to decide
whether to switch or stay.
| def setUp(self): | ||
| self.dependency = simulated.Stage("sstage_adaptive", "test", {"a"}) | ||
| self.dev = FixedPositionsActuator( | ||
| "stage_adaptive", "stage", | ||
| {"x": self.dependency}, "a", | ||
| {0: "pos0", 0.01: "pos1", 0.02: "pos2", | ||
| 0.03: "pos3", 0.04: "pos4", 0.05: "pos5"}, | ||
| cycle=0.06, | ||
| ) | ||
| # Provide a mock reference() on the dependency since simulated.Stage doesn't have one | ||
| self.dependency.reference = unittest.mock.MagicMock( | ||
| return_value=model.InstantaneousFuture() | ||
| ) | ||
|
|
||
| def tearDown(self): | ||
| self.dev.terminate() | ||
| self.dependency.terminate() | ||
|
|
||
| def _run_quick_reference_with_timing(self, t_premove: float, t_ref: float): | ||
| """ | ||
| Call _quick_reference() with mocked time.monotonic() so that the | ||
| reported pre-move duration is t_premove and the reference duration is t_ref. | ||
|
|
||
| :param t_premove: simulated pre-move duration in seconds | ||
| :param t_ref: simulated reference duration in seconds | ||
| """ | ||
| # time.monotonic() is called 4 times: start_premove, end_premove, start_ref, end_ref | ||
| side_effects = [0.0, t_premove, t_premove, t_premove + t_ref] | ||
| with unittest.mock.patch("odemis.driver.actuator.time.monotonic", | ||
| side_effect=side_effects): | ||
| self.dev._quick_reference() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/odemis/driver/test/actuator_test.py | sed -n '199,311p'Repository: delmic/odemis
Length of output: 6483
Add missing return type hints and docstrings to setUp/tearDown methods.
All methods are missing the -> None return type hint required by the coding guidelines. Additionally, setUp() and tearDown() are missing docstrings. The helper method _run_quick_reference_with_timing() and all test methods need the -> None return type annotation.
Methods requiring fixes:
setUp()(line 199): needs-> Noneand docstringtearDown()(line 213): needs-> Noneand docstring_run_quick_reference_with_timing()(line 217): needs-> None- All test methods (lines 231–311): need
-> None
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/odemis/driver/test/actuator_test.py` around lines 199 - 229, Update the
test methods to include explicit return type annotations and add missing
docstrings: add a "-> None" return type to setUp, tearDown,
_run_quick_reference_with_timing and all test_... methods, and add concise
docstrings to setUp and tearDown explaining their purpose (e.g., setUp prepares
a simulated Stage and FixedPositionsActuator; tearDown terminates them). Locate
the methods by name (setUp, tearDown, _run_quick_reference_with_timing and any
test_* functions) in actuator_test.py and apply the annotations and docstrings
consistently to satisfy the coding guidelines.
| def test_initial_state(self): | ||
| """Without explicit ref_start, auto mode is enabled and the first candidate is cycle * 0.1.""" | ||
| self.assertTrue(self.dev._ref_start_auto) | ||
| self.assertAlmostEqual(self.dev._ref_start, 0.06 * 0.1) | ||
| self.assertEqual(self.dev._ref_candidate_idx, 0) | ||
| self.assertEqual(self.dev._ref_candidate_times, [None, None]) |
There was a problem hiding this comment.
These assertions don't match _quick_reference()'s current state transitions.
src/odemis/driver/actuator.py only creates _ref_candidate_idx on the slow-path, and the fast-path sets _ref_start_auto = False at Lines 1560-1564. Either initialize/update that field in production or assert on _ref_start / _ref_candidate_times here, but as written these tests will fail against the current implementation.
Also applies to: 267-276, 302-311
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/odemis/driver/test/actuator_test.py` around lines 231 - 236, The test
assumes state set by _quick_reference() but the implementation only creates
_ref_candidate_idx on the slow-path and the fast-path clears _ref_start_auto;
update the tests to assert only the fields always set by both paths (e.g.
_ref_start and _ref_candidate_times) and remove assertions on
_ref_candidate_idx/_ref_start_auto, or alternatively modify the production
_quick_reference() to always initialize _ref_candidate_idx and consistently set
_ref_start_auto; apply the same change pattern to the other failing tests (the
blocks around the other mentioned ranges) and reference the symbols
_quick_reference, _ref_start, _ref_candidate_times, _ref_candidate_idx, and
_ref_start_auto when making edits.
pieleric
changed the title
Commit 159a10f (FixedPositionsActuator on rotation axis could take a long time to reference)
fixed a bug that caused the referencing from almost never happening on
cyclic actuators.
However, if the default ref_start value is on the wrong side of the
reference switch (for example because the actual axis is inverted), then
the referencing can take a long time. On some old METEOR, that can cause up to
50s delay to switch slot.
So adjust it in 2 ways:
from each sides of the reference switch, and afterwards it uses the fastest option.
This "smart" behaviour is skipped if the position is given in the mic file.
It can also detect that the first referencing was quick, and so never tries the other direction.