bluesky / ophyd

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

v2: Initial support for NTNDArray and correct timestamp -> timeStamp #1148

Closed Tom-Willemsen closed 1 year ago

Tom-Willemsen commented 1 year ago

Allow Ophyd V2 to read from an NTNDArray type pva variable.

At the moment it is using the standard array converter, which means that the array is always provided downstream as 1-D - it might be better to eventually do the reshaping into a correctly-shaped NTNDArray in the converter, but this is sufficient for my simple use-case at the moment and it wasn't entirely clear to me how to get at the metadata (i.e. shape) from within the current converter mechanism.

Also fixes a typo (?) of timestamp -> timeStamp, which I think would have affected every pva variable and was preventing me from reading array data over PVA. As far as I can tell from the various PVAccess specs/documents online, timeStamp is always the standard name that is used. Happy to be corrected on this if I'm wrong though...

rosesyrett commented 1 year ago

Seems reasonable to me, though I think we need a test. e.g. adding bits to the .db file here: https://github.com/bluesky/ophyd/blob/master/ophyd/v2/tests/test_records.db and checking that we can successfully extract the NTNDArray type from it.

I think @coretl will have some thoughts on the dimensions of these tables, so I think we ought to wait for his input on this.

callumforrester commented 1 year ago

Unsure why codecov is complaining as the lines affected haven't been touched by this PR, anyone have any ideas?

Tom-Willemsen commented 1 year ago

Seems reasonable to me, though I think we need a test. e.g. adding bits to the .db file here: https://github.com/bluesky/ophyd/blob/master/ophyd/v2/tests/test_records.db and checking that we can successfully extract the NTNDArray type from it.

Test added. Sorry I hadn't quite noticed how these were run previously!

I think @coretl will have some thoughts on the dimensions of these tables, so I think we ought to wait for his input on this.

From my perspective the shape isn't a blocker - I can get shape from elsewhere and reshape downstream. But long term it would be a nice-to-have for the shape to be correct here.

rosesyrett commented 1 year ago

Thanks for this. I'll see about adding some config for codecov to stop complaining when there are tiny changes (i.e. +/- 0.1%) to code coverage for the project/patch...

rosesyrett commented 1 year ago

I've made an issue about the codecov job failing; you can read it here: https://github.com/bluesky/ophyd/issues/1149

There's a PR to introduce tolerances here: https://github.com/bluesky/ophyd/pull/1150

Once that's merged we could rebase this on top of it, and then merge it in. Most likely this will happen tomorrow when the NSLS-2 ophyd v2 meeting takes plcae

coretl commented 1 year ago

Discussed this with @Tom-Willemsen in person and agreed that the output should be N-dimensional rather than the 1-dimensional array that is present in the PVA structure

Tom-Willemsen commented 1 year ago

Closing in favor of https://github.com/bluesky/ophyd-async/pull/19 now that ophyd-async has been moved to a different repo.