-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Create an instantly moving mock motor #57
Comments
@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. |
I think there should be 2 sorts of "not real" motors:
@DominicOram will 1. be sufficient to solve this? In which case I suggest we turn this ticket into "Make a slightly realistic SimMotor" |
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 |
@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? |
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. |
Yh, that's fine for me. Thank you! |
The primary use case is docs and tutorials, something you can poke in iPython to play around with bluesky plans |
Does #224 provide everything this issue requires? If so we can close it |
I think it does |
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. |
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:
I do worry I'm starting to describe dummy mode though... |
@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. However, it does seem useful to have a true mock (i.e. what we currently call |
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 |
I agree it's a separate issue, I don't think this one can be considered "done" until the interfaces of |
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. |
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? |
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:
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. |
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 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. |
@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 discussing with @coretl, the worry here is that injecting arbitrary functionality into something called e.g. 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? |
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 Apologies for being terse, I'm just keen to use this in |
Having re-read this thread, let me check I have the right understanding:
@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 @DominicOram what do you think? |
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. |
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 |
@DominicOram have you found any repeated patterns in mock motors that are worth bringing upstream? |
We have |
I think something like |
We normally use it as a function. In fact, when we had the similar with patch_motor(my_motor):
do_test() # Motor patched
# Motor now unpatched We also have the following for patching multiple motors: with ExitStack() as motor_patch_stack:
for motor in motors:
motor_patch_stack.enter_context(patch_motor(motor))
yield device A further step would even be: def find_all_motors(device) -> List[Motor]:
# recursively find all motor children of the device
def patch_all_motors(device):
with ExitStack() as motor_patch_stack:
for motor in find_all_motors(device):
motor_patch_stack.enter_context(patch_motor(motor))
yield device |
I learned something new today: How about a pair of context managers in
def motor_mocked_to_instantly_move(motor: Motor, initial_position: float = 0) -> contextlib.AbstractContextManager:
set_mock_value(motor.user_setpoint, initial_position)
set_mock_value(motor.user_readback, initial_position)
return callback_on_mock_put(
motor.user_setpoint,
lambda pos, *args, **kwargs: set_mock_value(motor.user_readback, pos),
)
def _find_all_motors(device: Device) -> Iterator[Motor]:
if isinstance(device, Motor):
yield device
for child in device.children:
yield from _find_all_motors(child)
def all_motors_mocked_to_instantly_move(device: Device) -> contextlib.AbstractContextManager:
stack = ExitStack()
for motor in _find_all_motors(device):
stack.enter_context(motor_mocked_to_instantly_move(device))
return stack
async def mock_dcm():
async with DeviceCollector():
dcm = ixx.dcm()
with all_motors_mocked_to_instantly_move(dcm):
yield dcm Bike shedding welcome... |
It may be useful for tests to have a mock motor that instantly moves it's readback to it's setpoint. This mock should live near the
Motor
object. Maybe it should be a function that mocks out anyMotor
children of a given device?This will be easier done when #49 is done.
The text was updated successfully, but these errors were encountered: