Create an instantly moving mock motor about ophyd-async HOT 25 OPEN

@DominicOram is this part of #94?

Unrelated I think. This issue is to make it easier to spin up mock motors for unit tests. #94 is I think to make more generic soft signal backends.

from ophyd-async.

Please can we keep this open while you're converting those tests to ophyd-async, then revisit it to see if we can bring any of the boilerplate upstream.

I wasn't against removing downstream boilerplate, but I didn't want to make a single set of logic that lived next to the simulation mode of EpicsMotor that became the way you used motors in simulation. As long as we have utility functions or fixtures like instantly_moving_motor or jittery_motor or following_error_motor rather than sim_motor then I'm happy to have that logic in ophyd-async.

from ophyd-async.

@DominicOram is this part of #94?

from ophyd-async.

I think there should be 2 sorts of "not real" motors:

1. A motor that looks realistic from the outside, i.e. takes time to move. There is ophyd_async.epics.demo.Mover that does this with a supporting IOC, and we should make ophyd_async.sim.demo.SimMotor that does the same thing and works with the PatternDetector created in #131
2. A motor that has no logic behind it, just a bunch of PVs that immediately accept the value that has been given to it. This will work for ophyd_async.epics.motion.Motor if connect(sim=True) is passed, with a slight issue, the readback will not update. This is what we want for tests, but I suspect not for this issue.

@DominicOram will 1. be sufficient to solve this? In which case I suggest we turn this ticket into "Make a slightly realistic SimMotor"

from ophyd-async.

1 will work for what we want but I'm not sure I agree with the taking time to move etc. as it's more complicated than needed. All we need is the minimum to be able to perform unit tests nicely e.g. be able to do a mv successfully. The easiest way to do this is to a motor where setting the setpoint immediately sets the readback.

from ophyd-async.

@coretl does 1. solve other use cases? If so @DominicOram, could you have a 1-style motor with the time to move set to 0?

from ophyd-async.

If nobody objects I'll work on creating a 1 style motor with a default time to move of 0. In full sim mode it would use the velocity to work out how long the move would take and update it throughout the move (probably), Could also take acceleration into account if there is any application for that.

from ophyd-async.

Yh, that's fine for me. Thank you!

from ophyd-async.

@coretl does 1. solve other use cases? If so @DominicOram, could you have a 1-style motor with the time to move set to 0?

The primary use case is docs and tutorials, something you can poke in iPython to play around with bluesky plans

from ophyd-async.

Does #224 provide everything this issue requires? If so we can close it

@DominicOram @coretl @gilesknap

from ophyd-async.

I think it does

from ophyd-async.

I've read through the previous comments and believe that we have implemented what the consensus is. Tom has approved this so I'll let Dom confirm if he is happy.

from ophyd-async.

Sorry to jump in on this when it's already been merged. It looks good and I think does match my usecase but I have some additional quires:

• The way this is written means it doesn't match the interface of the Motor e.g. there's no acceleration_time signal. This means if we want to replace all instances of Motor with SimMotor for our unit tests there may be places where things fail. Ideally we would have an inheritance structure that guarantees Motor and SimMotor match?
• Probably a new issue but it would be nice to have an automated way of getting a SimMotor when you connect a device that contains a Motor using sim=True. A general framework for this would be even better

I do worry I'm starting to describe dummy mode though...

from ophyd-async.

@DominicOram I'm on the fence...

The problem with dummy mode is not that exists or that it has complex dummy functionality, but that it has to be maintained separately and in addition to live mode. Each beamline has two sets of config that must be kept manually in-step and the dummy-ness/live-ness of each is arbitrary/defined by convention.
A "dummy mode" which is built automatically behind the scenes when you feed a live config into it (i.e. what you're suggesting) seems like a good improvement.

However, it does seem useful to have a true mock (i.e. what we currently call sim=True) and a "sim" with complex behaviour as different things for different purposes

from ophyd-async.

My requirement for this is that the complex behaviour should just be enough to get the device functional for most unit tests. The classic example is wire a setpoint to a readback. As with the non-zero velocity above, I don't want us to start going down the route of providing "realistic" simulations. If we want to do that we should do it in tickit

from ophyd-async.

I agree it's a separate issue, I don't think this one can be considered "done" until the interfaces of Motor and SimMotor match at least though (your first bullet point). @gilesknap Are you able to quickly make an amending PR since you're already in that space?

from ophyd-async.

Sure I can add acceleration.

My first try was derived from motor.@coretl insisted that this be independent of any epics classes, so this is the result.

from ophyd-async.

Acceleration was just an example, i think we ideally need to match the whole interface. What about having this and the motor derive from a common class that holds that interface?

from ophyd-async.

OK so if I understand you and your are talking about the ophyd-async device for the real epics motor then the missing properties are:

        self.max_velocity = epics_signal_r(float, prefix + ".VMAX")
        self.acceleration_time = epics_signal_rw(float, prefix + ".ACCL")
        self.precision = epics_signal_r(int, prefix + ".PREC")
        self.deadband = epics_signal_r(float, prefix + ".RDBD")
        self.motor_done_move = epics_signal_r(float, prefix + ".DMOV")
        self.low_limit_travel = epics_signal_rw(int, prefix + ".LLM")
        self.high_limit_travel = epics_signal_rw(int, prefix + ".HLM")

Are you saying that you need the actual functionality of these?

I now think the model that @coretl and @DominicOram have for what this is supposed to be may be very different. @coretl please can you comment on the above? Thanks.

from ophyd-async.

Yep, those are the ones. No, I don't think I need them to have any sensible behavior for now. I guess my use-case is that I would like the sim motor to be a drop in replacement for a real motor in a test for a plan, as much is reasonably practical. I appreciate that the motor may not behave correctly and so I may have to do some more clever mocking if the plans are too complicated but I would like the general case to work. Here are some example plans that are semi-realistic to what we're doing in Hyperion:

def my_plan(my_motor: Motor | SimMotor):
     yield from bps.mv(my_motor)

Which now works when we pass in sim motor but doesn't under a normally motor that is connected with sim=True. However, I would have to make my own sim device to be able to run:

def my_plan(my_motor: Motor | SimMotor):
    position_to_move_to = 100
    low_limit = yield from bps.rd(my_motor.low_limit_travel)
    new_pos = min(position_to_move_to, low_limit)

    max_velo = yield from bps.rd(my_motor.max_velocity)
    accel =  yield from bps.rd(my_motor.acceleration_time)
    fastest_we_can_get_to_position = # some maths here using max_velo and accel
    yield from bps.abs_set(my_detector.exposure_time, fastest_we_can_get_to_position)

Re-reading the above comments I think this is my fault for not being clear that this is what I wanted from the start, I'm happy to spin this into a new issue.

from ophyd-async.

@DominicOram let's see what @coretl thinks - my own feeling is that it would be OK to add in the other properties and even have them behave in a reasonably consistent fashion. However it then seems like maybe we just need to somehow tell Motor to move instantly when we specify sim mode rather than have a whole new thing.

from ophyd-async.

From discussing with @coretl, the worry here is that injecting arbitrary functionality into something called e.g. EpicsMotor (emphasis on the Epics) is a slippery slope. It requires the user to see the sim=True and just know that means class X will now behave like Y. I think I agree with this, I don't think it's DeviceCollector's job to inject any hidden functionality.

I wonder if this is actually dodal's job, so you can define a dummy version of each device factory for a beamline. It would give us a version of dummy mode that couldn't diverge too far from live mode. What do you think @DominicOram?

from ophyd-async.

I disagree but this is also going way outside the scope of this issue, which is my fault for derailing it. We're no where near actually implementing such a system so we can discuss in a follow on issue.

However, for this mock motor to be useful to me my requirement is that it needs to provide the same interface as EpicsMotor. I would suggest to keep that maintainable this and EpicsMotor should share a common parent class. If you reject my requirement then I would like to discuss it in person. If you reject my implementation suggestion then that's fine, implement how you would like, I'm not an ophyd-async maintainer.

Apologies for being terse, I'm just keen to use this in dodal/hyperion as it really cleans things up.

from ophyd-async.

Yep, those are the ones. No, I don't think I need them to have any sensible behavior for now. I guess my use-case is that I would like the sim motor to be a drop in replacement for a real motor in a test for a plan, as much is reasonably practical. I appreciate that the motor may not behave correctly and so I may have to do some more clever mocking if the plans are too complicated but I would like the general case to work. Here are some example plans that are semi-realistic to what we're doing in Hyperion:

def my_plan(my_motor: Motor | SimMotor):
     yield from bps.mv(my_motor)

Which now works when we pass in sim motor but doesn't under a normally motor that is connected with sim=True. However, I would have to make my own sim device to be able to run:

def my_plan(my_motor: Motor | SimMotor):
    position_to_move_to = 100
    low_limit = yield from bps.rd(my_motor.low_limit_travel)
    new_pos = min(position_to_move_to, low_limit)

    max_velo = yield from bps.rd(my_motor.max_velocity)
    accel =  yield from bps.rd(my_motor.acceleration_time)
    fastest_we_can_get_to_position = # some maths here using max_velo and accel
    yield from bps.abs_set(my_detector.exposure_time, fastest_we_can_get_to_position)

Re-reading the above comments I think this is my fault for not being clear that this is what I wanted from the start, I'm happy to spin this into a new issue.

Having re-read this thread, let me check I have the right understanding:

1. SimMotor is the bare minimum needed to have something that looks a bit like a generic motor that can be used in a scan for tutorials and testing clients of bluesky
2. EpicsMotor with connect(sim=True) is to be used for testing plans that normally take an EpicsMotor

@DominicOram I propose we refocus this issue on making 2. work for your use case. Does this work for you? In order to do that I would like to see what you want to test as well as the plan. For instance after #251 I can imagine something like:

def my_plan(my_motor: EpicsMotor):
    position_to_move_to = 100
    low_limit = yield from bps.rd(my_motor.low_limit_travel)
    new_pos = min(position_to_move_to, low_limit)
    max_velo = yield from bps.rd(my_motor.max_velocity)
    accel =  yield from bps.rd(my_motor.acceleration_time)
    fastest_we_can_get_to_position = # some maths here using max_velo and accel
    yield from bps.abs_set(my_detector.exposure_time, fastest_we_can_get_to_position)

@fixture
def mock_motor():
    with DeviceConnector(mock=True):
        motor = EpicsMotor("BLxxI-...")
    yield motor

def test_my_plan(mock_motor, mock_detector):
    set_mock_value(mock_motor.low_limit_travel, -40)
    set_mock_value(mock_motor.max_velocity, 12)
    set_mock_value(mock_motor.acceleration_time, 0.5)
    RE(my_plan(mock_motor))
    assert_mock_put_called_with(mock_detector.exposure_time, 0.45323434)  # whatever we expect that maths to make

None of that has required any logic to be applied to the motor. However this plan would need some logic applying to set readback from motor:

def move_and_return(my_motor: EpicsMotor, value: float) -> float:
    yield from bps.abs_set(my_motor, value)
    readback = yield from bps.rd(my_motor)
    return readback

We could do this in the fixture:

@fixture
def not_quite_there_motor():
    with DeviceConnector(mock=True):
        motor = EpicsMotor("BLxxI-...")
    # Make the motor not quite get to the endpoint
    set_mock_callback(motor.setpoint, lambda v: set_sim_value(motor.readback, v + 0.1)) 
    yield motor

def test_my_plan(not_quite_there_motor):
    ret = RE(move_and_return(not_quite_there_motor, 15))
    assert ret.plan_result == 15.1

The idea here being that the logic you want is added in the fixture, so you can make the motor do exactly what you want for the test being written.

My discussion with @callumforrester was about "what happens if there is some common logic that you need in many unit tests?" and that is where we discussed making these common fixtures (like not_quite_there_motor) available somewhere like in dodal.

@DominicOram what do you think?

from ophyd-async.

Our current solution looks like what I think you're suggesting https://github.com/DiamondLightSource/hyperion/blob/669f3ad6109020873fe9e3e56d9b7fcf077d4e22/tests/conftest.py#L252.

My original idea with this issue was to try and remove the need for the downstream boilerplate but if we want to just put it downstream that's fine. Happy to close this issue.

from ophyd-async.