bluesky / ophyd

hardware abstraction in Python with an emphasis on EPICS
https://blueskyproject.io/ophyd
BSD 3-Clause "New" or "Revised" License
49 stars 78 forks source link

Let SimSignalBackend accept npt.NDArray datatypes #1118

Closed rosesyrett closed 1 year ago

rosesyrett commented 1 year ago

solves https://github.com/bluesky/ophyd/issues/1117

rosesyrett commented 1 year ago

I would ideally like a test to check that, for the pva/ca case, a type of npt.NDArray also works as expected. However, I'm struggling thinking of how to do this we this would mean ophyd/v2/tests/test_record.db would need to output something like a npt.NDArray[np.float64].... This db file has waveforms which are tested in the form of TypedDicts (e.g. the Table object) but not quite the same as, for example, PC_OUT on a zebra, which is just a stream of floats (or an array of them).

rosesyrett commented 1 year ago

I've decided to change how SimSignalBackend works, to be more inline with the other backends, i.e. to have converters. In reality these converters don't actually need to 'convert' anything but it's a neat way to keep logic separated. I have kept methods like '.value' on the converters because I like the idea of the SimSignalBackend giving you a DisconnectedError if the converter hasn't been set properly, and the easiest way of doing this is by going through the converter for things like get_value and maybe even get_reading.

What do you think? @coretl and @evalott100

rosesyrett commented 1 year ago

Jobs will fail for now, there's a couple of things that still need sorting out.

evalott100 commented 1 year ago

What do you think?

I'll defer to Tom for that kind of decision (I haven't worked much with ophyd), but I'll follow the PR :slightly_smiling_face:

rosesyrett commented 1 year ago

getting some interesting failures in the tests... they seem unrelated to my change.

rosesyrett commented 1 year ago

just investigating why the CI job is failing... it shouldn't be. I'm unsure as to how this is happening as I can't recreate this on my machine.

rosesyrett commented 1 year ago

should be fixed now... :crossed_fingers:

rosesyrett commented 1 year ago

I've made the change you requested; I don't like it, because I don't understand why SimSignalbackend can't behave exactly like the other backends, and why it needs a non-synchronous method equivalent to a put method. my issue with this, is people may use the simulated backend in a way that they wouldn't do with an aioca/p4p one, so if they ever swap it out things fail.

coretl commented 1 year ago

I've made the change you requested; I don't like it, because I don't understand why SimSignalbackend can't behave exactly like the other backends, and why it needs a non-synchronous method equivalent to a put method. my issue with this, is people may use the simulated backend in a way that they wouldn't do with an aioca/p4p one, so if they ever swap it out things fail.

The issue is that sometimes (rarely) we want to test the case where 2 signals update on the backend and fire callbacks before our coroutine fires. This is only predictable if we can set 2 sim values in quick succession without another coroutine firing. This is only guaranteed if set_sim_value is sync. Using the sim backend is about being able to make predictable things that could happen unpredictably with the aioca/p4p ones (because of the inherent race conditions of multiple PVs updating over network sockets).

tacaswell commented 1 year ago

Very superficial question: do we want to put simulation related code in its own module rather than core?

rosesyrett commented 1 year ago

Very superficial question: do we want to put simulation related code in its own module rather than core?

Yes - although I think that should come as another PR in the form of refactoring anyways. There's a lot of stuff in core.py that should be separated out in my opinion. For example all the signals should probably go into a signals.py file instead of core.py, and there are plenty of what most people would consider 'utility' functions like wait_for_connection that should probably go in a utils.py, although I know there are mixed opinions about the code-cleanliness of the existence of a utils.py.

Either way I think this needs a broader discussion.