I24: get the beam centre from the #55
Conversation
Codecov Report
@@ Coverage Diff @@
## main #55 +/- ##
==========================================
+ Coverage 46.88% 47.56% +0.68%
==========================================
Files 19 19
Lines 3242 3267 +25
==========================================
+ Hits 1520 1554 +34
+ Misses 1722 1713 -9
📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today! |
noemifrisina
requested review from
Tom-Willemsen and
DominicOram
and removed request for
Tom-Willemsen
There was a problem hiding this comment.
See comment on DiamondLightSource/dodal#220 (review). I also think that the tests are testing slightly the wrong things. We don't need tests that confirm OAVParameters is reading the files and doing the calculations correctly, it's up to dodal to test that. We instead should mock out the function on OAVParamaters to give us a specific beam position and test that the ellipse gets drawn in the right place and the move on click gets calculated correctly.
| # Register clicks and move chip stages | ||
| def onMouse(event, x, y, flags, param): | ||
| if event == cv.EVENT_LBUTTONUP: | ||
| beamX, beamY = get_beam_centre_from_oav() | ||
| if event == cv.EVENT_LBUTTONUP: # cv.EVENT_LBUTTONUP is an int! (=4) |
There was a problem hiding this comment.
Should: What does this comment mean? I'm not sure what it adds?
There was a problem hiding this comment.
To the code nothing, a note from trying to test this I forgot to remove
| # Create a video caputure from OAV1 | ||
| cap = cv.VideoCapture("http://bl24i-di-serv-01.diamond.ac.uk:8080/OAV1.mjpg.mjpg") | ||
| cap = cv.VideoCapture(oav1) | ||
| # cap = cv.VideoCapture("http://bl24i-di-serv-01.diamond.ac.uk:8080/OAV1.mjpg.mjpg") |
There was a problem hiding this comment.
Should: Can you remove commented out code please?
| @patch("mx_bluesky.I24.serial.fixed_target.i24ssx_moveonclick.caput") | ||
| @patch("mx_bluesky.I24.serial.fixed_target.i24ssx_moveonclick.get_beam_centre_from_oav") | ||
| def test_onMouse__gets_beam_position_and_sends_correct_str(fake_beam_pos, fake_caput): | ||
| fake_beam_pos.side_effect = [(15, 10)] | ||
| onMouse(cv.EVENT_LBUTTONUP, 0, 0, "", "") | ||
| assert fake_caput.call_count == 2 | ||
| fake_caput.assert_has_calls( | ||
| [ | ||
| call(ANY, "#1J:-90"), | ||
| call(ANY, "#2J:60"), | ||
| ] | ||
| ) |
There was a problem hiding this comment.
Should: I think it would be good to parametrize this into (beam_pos, expected_J1, expected_J2) and have half a dozen or so values
| def test_get_beam_centre( | ||
| zoom_level, | ||
| expected_beamX, | ||
| expected_beamY, | ||
| fake_oav: OAV, | ||
| mock_oavparams: OAVParameters, | ||
| ): | ||
| fake_oav.zoom_controller.level.sim_put(zoom_level) | ||
| beamX, beamY = _get_beam_centre(fake_oav, mock_oavparams) | ||
| assert beamX == expected_beamX | ||
| assert beamY == expected_beamY |
There was a problem hiding this comment.
Should: I think we can remove this test, dodal is doing it for us. With it removed we can then remove mock_oavparams and all the configs in conftest
Fixes #42
Instead of using hard coded values that need to be updated manually, get the beam centre from
display.configurationsin GDA using dodal devices.Link to dodal PR: PR220