PVPositionerSoftDone should set done False at start of move

[prjemian]

• close #916
• close #810

[prjemian]

Tests pass locally but not in CI here. Setting PR back to draft for now.

[prjemian]

I'd like to merge in the next day (so apstools can be released on Friday). Any objections to merging by the end of business today?

[prjemian]

I'll wait until your report tomorrow. Or end of Thursday comes.

[gfabbris]

Tested using a Lakeshore 340. The original problem is fixed. However, I found another issue: if a ramp is used, the device marks the movement as done even though it is not. But I think I know where the problem is, the target signal is not being checked by inposition (this target is used when ramping because the readback value of the setpoint follows the ramp, being slowly changed by EPICS). Looks like all we need is to change:

https://github.com/BCDA-APS/apstools/blob/9e8a79398bb432523508c1113159a219fe15d957/apstools/devices/positioner_soft_done.py#L202

to:

sp = self.setpoint.get() if self.update_target is False else self.target.get()

(this worked for the lakeshore)

[prjemian]

Trying that solution locally. That will need more investigation. Many unit tests (in apstools/devices/tests/test_positioner_soft_done.py) fail with that change.

[prjemian]

FWIW, here's the output from pytest covering the failed tests:

The first error is a Type Error:

>       inpos = math.isclose(rb, sp, abs_tol=tol)
E       TypeError: must be real number, not str

apstools/devices/positioner_soft_done.py:204: TypeError

so it might be easy to resolve.

[prjemian]

On debugging, the assignment sp = self.setpoint.get() if not self.update_target else self.target.get() must be a bit more nuanced. Most of the cases that fail happen when self.target.get() is "None". (tracking that down next).

One other case fails differently:

FAILED apstools/devices/tests/test_positioner_soft_done.py::test_move_and_stopped_early - AssertionError: p.name='pos'  rb=6.88000 sp=6.88000 tol=0.0001  dt=0.2817s  p.done=Signal(name='pos_done', parent='pos', value=False, timestamp=1...

[prjemian]

The test failure is in the inposition() property. Clearly, if the target is the wrong Python type, then inposition must be False. Seems to be an edge case in unit testing, though.

[prjemian]

With that change, still needs more help:

>       assert pos.inposition, f"{pos.position=} {target=}"
E       AssertionError: pos.position=0.0 target=0
E       assert False
E        +  where False = PVPositionerSoftDoneWithStop(prefix='gp:gp:', name='pos', settle_time=0.0, timeout=None, read_attrs=['readback', 'setpoint'], configuration_attrs=['done', 'tolerance', 'target'], limits=None, egu='').inposition

[prjemian]

root cause: target is never changed from its initial default: https://github.com/BCDA-APS/apstools/blob/9e8a79398bb432523508c1113159a219fe15d957/apstools/devices/positioner_soft_done.py#L104

This clause is the only place in the code where target is updated: https://github.com/BCDA-APS/apstools/blob/9e8a79398bb432523508c1113159a219fe15d957/apstools/devices/positioner_soft_done.py#L215-L219

[prjemian]

The change might be different than you imagined. With the additional target field, there is some special handling of the setpoint. To me, this drives the addition of a custom subclass of EpicsSignal that overrides the standard get() and put() methods. The override considers both target and update_target attributes of the PVPositionerSoftDone() class. I believe this brings the special case handling into the best spot to resolve it. Locally, the unit tests pass with this change.

Also, changed the default value of the target signal from "None" to None, which is much easier to handle as a default.

[prjemian]

All tests _in test_positioner_soft_done.py_ ran successfully. Now checking the other tests locally, since CI failed on some of them.

[prjemian]

@gfabbris Can you test this branch again? I'm resetting your approval pending the outcome of your retests.

[gfabbris]

I don't understand the reason to refactor the setpoint signal. I'd prefer to have the setpoint signal return the EPICS readback value rather than the target.

[prjemian]

[prjemian]

I'm not certain that the wait=True kwarg is necessary.

[prjemian]

The lakeshore test suite cannot test the function of the controller. No Lakeshore controller is available on GitHub.

[prjemian]

I don't understand the reason to refactor the setpoint signal. I'd prefer to have the setpoint signal return the EPICS readback value rather than the target.

Wasn't that your suggestion?

Also, I see why the default value for target was not None. That type of Python object cannot be serialized as JSON and bluesky raises a ValueError. I've noted this in the code and refactored with a TARGET_UNDEFINED symbol.

[prjemian]

Also, unrelated change:

apstools/plans/tests/test_alignment.py:0: PytestCollectionWarning: cannot collect test class 'TestParameters' because it has a __new__ constructor (from: apstools/plans/tests/test_alignment.py)

Just change the name of the parameters class and pytest is happy.

[prjemian]

When the positioner setpoint is defined, it must set the positioner's done moving to False. The existing code for PVPositionerSoftDone needed this same handling in multiple places. The cb_setpoint() method seemed logical. But that would not be the correct place since cb_setpoint() is called in reaction to any EPICS CA monitor, not just from this Python class.

The logical place to set done moving to False is in the setpoint's put() method. That needs to happen in a custom subclass.

Is there another, better way to do this?

[prjemian]

If we keep the setpoint subclass, can we apply the same thing here? https://github.com/BCDA-APS/apstools/blob/10788f24c77cf9c6ff7f85b8423c0d4d182b1333/apstools/devices/lakeshore_controllers.py#L43

[prjemian]

Or is it time to act on this? https://github.com/BCDA-APS/apstools/blob/10788f24c77cf9c6ff7f85b8423c0d4d182b1333/apstools/devices/lakeshore_controllers.py#L17-L18

[gfabbris]

In the PVPositionerSoftDone I personally thing it'd be best to avoid the need for special classes for the setpoint or readback because I think the idea is to make it as general as possible, and sometimes we may want to redefine these signals (as in the Lakeshores).

I don't fully understand why it was failing the unit tests, did you mean to say that the main issue was defining the target signal as None? If that was the issue, then I find the previous solution better (with sp = self.setpoint.get() if not self.update_target else self.target.get() and setpoint being an EpicsSignal).

[prjemian]

If that was the issue, then I find the previous solution better (with sp = self.setpoint.get() if not self.update_target else self.target.get() and setpoint being an EpicsSignal).

failed many tests

[prjemian]

These failures.

[gfabbris]

But do you understand why it failed?

[prjemian]

As much as I want this branch in the next release (due tomorrow), it's obvious this branch will take more work.

[prjemian]

Prioritizing agreement over urgency.

[prjemian]

I believe I understand why it failed and responded appropriately. Let's take a breath here and attempt a refactor without the custom subclass.

[gfabbris]

Based on my limited understanding of unit testing, maybe I understand now why the test_position_sequence_pos failed but the actual lakeshore device works.

The unit test runs .put to the setpoint and readback of the PVPositionerSoftDone, and then asserts if the device is inposition. This will fail if update_target is True because the target is only updated when one runs a move in the whole device (which is done when moving the temperature in the Lakeshore). Although I think this behavior is ok, it would be better if inposition reported the correct state regardless of the way that the setpoint was changed. Can it be done with a subscription to the SUB_SETPOINT that updates the target signal?