Document when to make a Device class extend Movable

[DiamondJoseph]

Increasingly our new devices are becoming StandardReadables with a structure of signals underneath them- this is good, easy to understand and a vote in favour of the Standardness of StandardReadable.
Sometimes, these devices have a few settable fields, and it is tempting to define a set method on the type to allow setting them in one step.

e.g. Backlight, CurrentAmp, BimorphMirror

I think it is useful to document when defining a set method is a value add, and when it is added boilerplate that makes using the device or writing plans for the device more complex without benefit.

I think in the cases where the set method is constructing a dict or dataclass mapping directly to the underlying signals, using this set method requires constructing that dict or dataclass in the plan. If this does not lead to a speedup of the set (e.g. the previously believed behaviour of the Bimorph, that all signals can be set, then the move triggered for an (N-1)fold speedup for N signals), I do not see the value of the set method, but the method must be documented, tested and maintained.

If there is a limited set of valid positions (e.g. captured as an Enum, or a float range) that require several axes in co-ordination, then the set method is useful.

As we move towards using ScanSpec to describe trajectory scans, and those specs are defined in terms of Movable[float]s, having plans that are used to accessing abs_set(device.field, 5) rather than abs_set(device, {"field": 5}) may make understanding plan flow better?

Acceptance Criteria

• A decision has been documented- perhaps in an ADR?
• The bimorph set has either been removed, or the behaviour fixed

[DiamondJoseph]

@DominicOram @Relm-Arrowny, @callumforrester says this is something you're interested in.

[callumforrester]

Definitely think we should document it, possibly upstream in ophyd-async, paging @coretl

[coretl]

I will gladly accept docs.

My opinion is that we should only add a set method if it is obvious what it means to set the Device. I can see that it makes sense to abs_set(Backlight, BacklightPosition.OUT), but I'm not so sure about abs_set(SR570, SR570GainToCurrentTable.SEN_1)...

Agree with @DiamondJoseph's position on using primitives rather than dataclass for the set method wherever possible.

[stan-dot]

for a 4d Movable like this:

class TablePosition(BaseModel):
    x: float
    y: float
    z: float | None = None
    theta: float | None = None

then how to set the mock correctly?

@pytest.fixture
async def table() -> Table:
    """Fixture to set up a mock Table device using DeviceCollector."""
    async with DeviceCollector(mock=True):
        table = Table(prefix="MIRROR:")

    def propagate_status(value: float, *args, **kwargs):
        set_mock_value(table.x.user_readback, value.x)
        set_mock_value(table.y.user_readback, value.y)
        set_mock_value(table.z.user_readback, value.z)
        set_mock_value(table.theta.user_readback, value.theta)

    callback_on_mock_put(table.x.user_readback, propagate_status)
    callback_on_mock_put(table, propagate_status)
    callback_on_mock_put(table, propagate_status)
    callback_on_mock_put(table, propagate_status)
    return table

not obvious how to proceed here

[DominicOram]

Sometimes, these devices have a few settable fields, and it is tempting to define a set method on the type to allow setting them in one step.

I don't think the backlight is an example of this. What we're trying to solve there is that it's a universal requirement that when you move the backlight out you should turn it off and when you move it in you should turn it on. To the point where if we didn't have this we would inevitably have the first question from the scientist be "I did abs_set(backlight.position, IN), why is my sample not backlit?".

I think therefore that the usecase for set is:

• You are doing some logic that a user almost always wants to do
• As you said, you get a speed up for setting multiple things at once

I actually think the CurrentAmp falls under this too, though I would refactor the class quite a bit to make that more obvious. For the bimorph example, assuming no speed up by setting multiple channels I would push the set logic into each BimorphMirrorChannel so I can do bimorp.channels[0].set(10.3) and it handles the complex logic for me.

[Relm-Arrowny]

My opinion is that we should only add a set method if it is obvious what it means to set the Device. I can see that it makes sense to abs_set(Backlight, BacklightPosition.OUT), but I'm not so sure about abs_set(SR570, SR570GainToCurrentTable.SEN_1)...

I guess abs_set(SR570, SR570GainToCurrentTable.SEN_1) does look crazy and it may work but the intention is for user to do this
abs_set(SR570, 1e3), where 1e3 is the current amplifier gain, it does behave like bimorph, it check if the value is allowed for the device before setting.

[DiamondJoseph]

...I don't think the backlight is an example of this ... there is that it's a universal requirement that when you move the backlight out you should turn it off and when you move it in you should turn it on

To me this is included in the "limited set of valid positions" sensible set method- there are 2 actually valid positions: (In, On) and (Out, Off).

I think for the CurrentAmp we just need to add type hints to the set method so it's clear what's going on: mapping from an allowed set of float Literal[1e3, 2e3, 5e3, 1e4, 2e4, 5e4, 1e5, 2e5, 5e5, 1e6, 2e6, 5e6, 1e7, 2e7, 5e7, 1e8, 2e8, 5e8, 1e9, 2e9, 5e9, 1e10, 2e10, 5e10, 1e11, 2e11, 5e11, 1e12] to what that means in the coarse/fine gain

[callumforrester]

Taking (what I think is) my summary of this thread in #1030, the docs should say the following:

• We favour set being used to assign primitive values rather than documents/dataclasses
• set is most useful if it has an obvious meaning, where a device is associated with a singular value (e.g. a motor)
• It is sometimes useful to streamline an operation that a user does a lot
• Occasionally there are specific requirements for making "setting" a device into an atomic operation. This is also roughly consistent with how dodal is today, or at least its direction of travel.
• Generally you shouldn't implement set on a device (collection of signals) unless it can be justified against the above criteria.

Anyone has anything else/disagrees with anything let me know!

[stan-dot]

We favour set being used to assign primitive values rather than documents/dataclasses

using set when a device is a clear vector in physical space - for a composite motor also has an obvious meaning

[coretl]

We favour set being used to assign primitive values rather than documents/dataclasses

using set when a device is a clear vector in physical space - for a composite motor also has an obvious meaning

Yes, although we are not settled on whether the input in this case should be a TypedDict or dataclass:
bluesky/ophyd-async#661

[stan-dot]

re: TypedDict

is this the right link?

it only mentions it twice?

from typing import Generic, TypedDict, TypeVar, get_args


  class TransformArgument(TypedDict, Generic[SignalDatatypeT]):
      pass

what aspect is the concern? performance, DX?

[coretl]

That is correct.

Concerns are how nicely it plays with typed and untyped usage. This is on hold at the moment but I thought I would mention it as it seemed relevant.