554 tidy oav #584

d-perl · 2023-03-16T09:26:50Z

Fixes #554

Link to dodal PR (if required): #25

To test:

Check organisation/naming of oav code is sensible/coherent
Run tests with both artemis and dodal branches

and combined grid detection and scan

DominicOram

Thanks for making a start on this but I think it still needs more cleaning up from prototyping. I think to merge it we need at least a unit test against some fake devices to confirm the plan runs through. Can you also address all the musts.

Can you write up issues for the other comments if you don't address them here? I also think this doesn't address all of the things on the issue it links. I have ticked the ones I think it fixes, can you split the others out into new issues?

DominicOram · 2023-03-27T14:12:29Z

src/artemis/experiment_plans/experiment_registry.py

 EXPERIMENT_TYPES = Union[GridScanParams, RotationScanParams]
 PLAN_REGISTRY: Dict[str, Dict[str, Callable]] = {
+    "full_grid_scan": {
+        "setup": full_grid_scan.create_devices,


Must: Without all these devices in S03 yet this now means that we can't start artemis on a developer machine any more. Not sure what the best solution is here, get some dummy devices into S03 ideally...

src/artemis/device_setup_plans/setup_oav.py

src/artemis/experiment_plans/full_grid_scan.py

src/artemis/experiment_plans/oav_grid_detection_plan.py

DominicOram · 2023-03-27T14:26:07Z

src/artemis/experiment_plans/oav_grid_detection_plan.py

+    width = 600
+    box_size_microns = 20


Should: These should probably come in as parameters?

DominicOram · 2023-03-27T14:26:49Z

src/artemis/experiment_plans/oav_grid_detection_plan.py

+    yield from bps.abs_set(oav.snapshot.input_pv, parameters.input_plugin + ".CAM")
+    yield from bps.abs_set(oav.mxsc.enable_callbacks_pv, 0)


Should: I think these need to be in a finally clause

DominicOram · 2023-03-27T14:29:57Z

src/artemis/experiment_plans/full_grid_scan.py

+    detector_motion = Det("BL03I", name="detector_motion")
+    backlight = Backlight("BL03I-EA-BL-01:", name="backlight")


Should: For the fgs plan we're passing the beamline in as a param, should probably do the same here

DominicOram · 2023-03-27T14:32:22Z

src/artemis/experiment_plans/full_grid_scan.py

+def wait_for_det_to_finish_moving(detector: Det, timeout=2):
+    LOGGER.info("Waiting for detector to finish moving")
+    SLEEP_PER_CHECK = 0.1
+    times_to_check = int(timeout / SLEEP_PER_CHECK)
+    for _ in range(times_to_check):
+        detector_state = yield from bps.rd(detector.shutter)
+        detector_z_dmov = yield from bps.rd(detector.z.motor_done_move)
+        LOGGER.info(f"Shutter state is {'open' if detector_state==1 else 'closed'}")
+        LOGGER.info(f"Detector z DMOV is {detector_z_dmov}")
+        if detector_state == 1 and detector_z_dmov == 1:
+            return
+        yield from bps.sleep(SLEEP_PER_CHECK)
+    raise Exception("Detector not finished moving")


Should: This is much like the waiting for fast grid scan to be valid, e.g. messy and horrible. I think we need to put it in the device side probably? Certainly refactor it to be more Bluesky idiomatic

DominicOram · 2023-03-27T14:51:16Z

src/artemis/utils/oav_utils.py

+    Returns the scale of the image. The standard calculation for the image is based
+    on a size of (1024, 768) so we require these scaling factors.


Should: This comment is not correct in that we don't use the scaling factors

- oav cleanup into finally - update for pv names - factor out params

d-perl · 2023-03-30T09:31:17Z

Fair enough, in that case, I'd like to get #595 merged first, since changes there will affect the work that needs to be done here quite a lot

codecov · 2023-04-06T09:32:27Z

Codecov Report

Merging #584 (5486d9d) into main (89ada29) will increase coverage by 8.32%.
The diff coverage is 94.79%.

@@            Coverage Diff             @@
##             main     DiamondLightSource/hyperion#584      +/-   ##
==========================================
+ Coverage   81.68%   90.01%   +8.32%     
==========================================
  Files          30       32       +2     
  Lines        1376     1412      +36     
==========================================
+ Hits         1124     1271     +147     
+ Misses        252      141     -111

Impacted Files	Coverage Δ
src/artemis/utils/utils.py	`100.00% <ø> (ø)`
src/artemis/experiment_plans/full_grid_scan.py	`89.79% <89.79%> (ø)`
...rtemis/experiment_plans/oav_grid_detection_plan.py	`96.15% <96.15%> (ø)`
src/artemis/device_setup_plans/setup_oav.py	`97.43% <97.43%> (ø)`
...rc/artemis/experiment_plans/experiment_registry.py	`93.75% <100.00%> (-6.25%)`	⬇️
...rc/artemis/experiment_plans/fast_grid_scan_plan.py	`82.43% <100.00%> (-4.06%)`	⬇️
...ernal_interaction/callbacks/fgs/zocalo_callback.py	`98.46% <100.00%> (ø)`
...emis/external_interaction/ispyb/ispyb_dataclass.py	`100.00% <100.00%> (ø)`
...temis/external_interaction/ispyb/store_in_ispyb.py	`96.92% <100.00%> (ø)`
.../external_interaction/zocalo/zocalo_interaction.py	`97.14% <100.00%> (ø)`
... and 1 more

... and 1 file with indirect coverage changes

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

DominicOram

Looks good, thanks. I tried to spin out as much as possible into other issues, what's left is things we can hopefully quickly fix now

DominicOram · 2023-05-11T14:58:05Z

src/artemis/__main__.py

    ) = cli_arg_parse()

    artemis.log.set_up_logging_handlers(logging_level, dev_mode)
+    dodal_logging_setup(logging_level, dev_mode)


Should: I don't think this is needed, artemis.log.set_up_logging_handlers calls it for you

DominicOram · 2023-05-11T14:58:52Z

src/artemis/device_setup_plans/setup_oav.py

+    # Set the python file to use for calculating the edge waveforms
+    current_filename = yield from bps.rd(oav.mxsc.filename)
+    if current_filename != filename:
+        LOGGER.info(f"Current python file is {current_filename}, setting to {filename}")


Nit: Message should mention that this is the coming form the OAV, it's not clear without context

src/artemis/experiment_plans/full_grid_scan.py

src/artemis/experiment_plans/tests/test_grid_detection_plan.py

DominicOram · 2023-05-11T15:55:47Z

src/artemis/system_tests/test_main_system.py

        mock_setup.assert_called_once()


+@pytest.mark.skip("fixed in 595")


Should: In #595? This is already merged, so we should remove this skip

DominicOram · 2023-05-11T15:58:51Z

src/artemis/experiment_plans/oav_grid_detection_plan.py

+        min_y = np.min(top_edge[top_edge != 0])
+        max_y = np.max(bottom_edge[bottom_edge != full_oav_image_height_px])
+        LOGGER.info(f"Min/max {min_y, max_y}")
+        # if top and bottom empty after filter use the whole image (ask neil)


Should: Neil said yes, we can remove the comment to ask him

src/artemis/experiment_plans/oav_grid_detection_plan.py

DominicOram · 2023-05-12T11:01:20Z

src/artemis/experiment_plans/full_grid_scan.py

    }

-    oav_params = OAVParameters("xrayCentring")
+    oav_params = OAVParameters("loopCentring", **oav_param_files)


Should: I think there is an issue using loopCentring here, spun out into #653

DominicOram

Great, thanks for the additional tests

DominicOram and others added 4 commits March 14, 2023 10:37

(#554) Initial OAV grid determination tested on beamline

03a4a0d

implement oav grid detection plan

adbd939

and combined grid detection and scan

separate out some utils

7b911bb

update pv names and oav location

d6fea90

d-perl mentioned this pull request Mar 16, 2023

Restructure OAV device organisation and naming (Artemis554) DiamondLightSource/dodal#25

Merged

d-perl added 2 commits March 16, 2023 15:01

add dodal version dep

729a9bb

fix test for multiple device instantiation

fb7eec2

DominicOram suggested changes Mar 27, 2023

View reviewed changes

DominicOram reviewed Mar 27, 2023

View reviewed changes

tidy

0d49dbe

- oav cleanup into finally - update for pv names - factor out params

d-perl marked this pull request as draft March 30, 2023 09:30

Merge branch 'main' into 554_tidy_oav

9a93376

d-perl added 15 commits April 24, 2023 12:18

Merge branch 'main' into 554_tidy_oav

2803df5

left over from merge

500ffcf

fix imports

d591af8

fix gull grid scan reg entry

8587609

skip test fixed in other branch

2df743a

remove unused import

0dd62c2

tidy device creation

1d3887c

tidy grid detection plan

33238eb

skeletons for tests

f90bd3c

flesh out grid detection test plan

029ff36

update dodal req

6264c81

fix formatting error

ce3fae7

Merge branch 'main' into 554_tidy_oav

a596c80

update dodal req

08e35f8

get rid of somehow tracked test files

e37337a

d-perl force-pushed the 554_tidy_oav branch from a2e1441 to e37337a Compare May 11, 2023 08:51

patch test oav config files

fd20b2c

d-perl marked this pull request as ready for review May 11, 2023 09:24

d-perl requested a review from DominicOram May 11, 2023 09:24

d-perl added 4 commits May 11, 2023 12:40

skip broken logging test

a07ce23

make test config files global, use for whole test

50a243e

skip broken logging test

1843a7e

inst oavparams with local config instead of patch

afe95a3

DominicOram suggested changes May 11, 2023

View reviewed changes

d-perl added 11 commits May 11, 2023 17:34

review changes

4061399

Merge branch 'main' into 554_tidy_oav

5d582a1

delete unused plan and add some tests

101b804

update dodal req

2494e19

fix default centring context and add test

1472740

fix test for CI

96bfc64

fix default param files

78b6b2b

make keyword names consistent

c616c40

update dodal req

06c984c

use argument instead of constant default

6c573f0

slightly nicer syntax

5486d9d

DominicOram mentioned this pull request May 12, 2023

Use xray centring params for grid detection #653

Closed

DominicOram reviewed May 12, 2023

View reviewed changes

DominicOram approved these changes May 12, 2023

View reviewed changes

DominicOram merged commit 4812b1f into main May 12, 2023

DominicOram deleted the 554_tidy_oav branch May 12, 2023 11:02

		yield from bps.abs_set(oav.snapshot.input_pv, parameters.input_plugin + ".CAM")
		yield from bps.abs_set(oav.mxsc.enable_callbacks_pv, 0)

		detector_motion = Det("BL03I", name="detector_motion")
		backlight = Backlight("BL03I-EA-BL-01:", name="backlight")

		Returns the scale of the image. The standard calculation for the image is based
		on a size of (1024, 768) so we require these scaling factors.

		mock_setup.assert_called_once()


		@pytest.mark.skip("fixed in 595")

554 tidy oav #584

554 tidy oav #584

Uh oh!

Conversation

d-perl commented Mar 16, 2023

To test:

Uh oh!

DominicOram left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d-perl commented Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Apr 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

DominicOram left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DominicOram left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

d-perl commented Mar 30, 2023 •

edited

Loading

codecov bot commented Apr 6, 2023 •

edited

Loading