129 edge detection

[Tom-Willemsen]

Initial dodal/ophyd device for pin-tip detection on I23:

• Closes i23: implement optical edge detection algorithm in dodal device #129
• Closes Allow using both Ophyd v1 & v2 devices concurrently #144

Notes:

• The edge detection code in edge_detect_utils.py is a refactored version of /dls_sw/prod/R3.14.12.7/support/adPython/2-1-11/adPythonApp/scripts/adPythonMxSampleDetect.py. Main changes:
  • Py3 compatibility
  • Use numpy for array operations (for efficiency)
  • Add unit tests
  • Various changes to make the API "nicer"
• Using ophyd V2 as I'm using PVA to pull the array data.
  • Needs v2: Initial support for NTNDArray and correct timestamp -> timeStamp bluesky/ophyd#1148 to be used for initial support of NTNDArray EPICS PVA datatype, and a minor patch of timestamp -> timeStamp (the latter being the EPICS standard).
  • Note that due to the absence of PVA gateways currently, if you want to test this you must do it on a beamline workstation. I've been using i02-1 or i03 when their cameras are switched on.

[Tom-Willemsen]

Draft at the moment (due to necessary ophyd PR not done yet), but would appreciate any feedback on the approach so far.

[DominicOram]

Great, thank you, mostly quite minor comments in code. Haven't managed to test on a real device yet but looks like it should work. I think we should change the name from pin_tip_detection, it does more than that in that it detects the bounding edge of the pin too. Maybe something like pin_image_recognition? Can we also make an artemis issue to switch over i03 to using this?

[codecov]

Codecov Report

Merging #143 (6552557) into main (e4501fc) will decrease coverage by 1.20%.
Report is 2 commits behind head on main.
The diff coverage is 79.92%.

❗ Current head 6552557 differs from pull request most recent head be318f9. Consider uploading reports for the commit be318f9 to get more accurate results

@@            Coverage Diff             @@
##             main     #143      +/-   ##
==========================================
- Coverage   89.25%   88.06%   -1.20%     
==========================================
  Files          62       65       +3     
  Lines        2047     2295     +248     
==========================================
+ Hits         1827     2021     +194     
- Misses        220      274      +54     

Files Changed	Coverage Δ
src/dodal/beamlines/i03.py	87.14% <ø> (ø)
src/dodal/beamlines/i23.py	0.00% <0.00%> (ø)
src/dodal/beamlines/i24.py	97.05% <ø> (ø)
...l/devices/oav/pin_image_recognition/manual_test.py	0.00% <0.00%> (ø)
...c/dodal/devices/oav/pin_image_recognition/utils.py	74.03% <74.03%> (ø)
src/dodal/beamlines/beamline_utils.py	97.82% <96.55%> (-2.18%)	⬇️
...odal/devices/oav/pin_image_recognition/__init__.py	98.79% <98.79%> (ø)
src/dodal/utils.py	100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[rosesyrett]

This is a big change; Most of my comments are to do with minor tweaks to make things more pythonic and questions about where things should live/if we could do things in slightly different ways to have slightly cleaner code.

Importantly, it's clear that this hasn't yet been tested in earlier versions of python, especially as the CI for python3.9 is failing due to type annotation issues.

You have two options in this regard:

1. continue to use python3.10 type annotations like dict and | (instead of 'Union') except add from future import __annotations__ at the top of all files that use these
2. Don't use those things, and instead use (e.g.) Dict and Union. With this option, to get rid of the CI error you'd need to import some things from typing_extensions. TypeDicts are a known example of this.

[DominicOram]

Thanks, sorry for adding more comments to an already dragging PR. Hopefully just some quick ones now

[DominicOram]

Should: Is this true? I thought it returned -1 if the sample was completely offscreen and the edge if the sample extended across the whole screen?

[Tom-Willemsen]

Have clarified comment, I think it's just potential ambiguity between "sample is off-screen" and "tip of the sample is off-screen". Hopefully new wording is clearer.

[rosesyrett]

Thanks very much for responding to my comments. They weren't all easy to deal with and I appreciate your responses 👍

I'd still wait for Dom's go ahead because my opinion on this is purely from a code readability standpoint

[DominicOram]

Sorry, just one more comment, promise...

[DominicOram]

Great, thank you!