Add deadtime optimisation by olliesilvester · Pull Request #755 · DiamondLightSource/hyperion

olliesilvester · 2023-06-22T14:21:03Z

Fixes #754

Link to dodal PR (if required): DiamondLightSource/dodal#102

To test:

Confirm deadtime optimisation logic is the same of that in the GDA script mx-config/scripts/attenuation_optimiser.py, in the run_xspress3mini_optimisation_routine()
Confirm new tests run and are sensible

codecov · 2023-06-22T14:45:39Z

Codecov Report

Merging #755 (47b245a) into main (473c4c8) will decrease coverage by 1.63%.
The diff coverage is 71.62%.

❗ Current head 47b245a differs from pull request most recent head 93e800e. Consider uploading reports for the commit 93e800e to get more accurate results

@@            Coverage Diff             @@
##             main     DiamondLightSource/hyperion#755      +/-   ##
==========================================
- Coverage   94.19%   92.56%   -1.63%     
==========================================
  Files          37       37              
  Lines        1808     1869      +61     
==========================================
+ Hits         1703     1730      +27     
- Misses        105      139      +34

Impacted Files	Coverage Δ
...emis/experiment_plans/optimise_attenuation_plan.py	`71.42% <71.62%> (-23.02%)`	⬇️

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

olliesilvester · 2023-06-23T11:07:15Z

+            )
+        )
+
+    if upper_limit < lower_limit:


I think this check is implied in the if statement before, so I will remove this

DominicOram

Good work, thank you. Some comments in code

DominicOram · 2023-06-29T10:50:16Z

+def deadtime_is_transmission_optimised(
+    direction, deadtime, deadtime_threshold, transmission
+) -> tuple[bool, bool]:
+    flip_direction = False
+    if direction:
+        if deadtime >= deadtime_threshold:
+            flip_direction = True
+            return False, flip_direction
+        else:
+            # The 0.9 is hardcoded in GDA
+            if transmission >= 0.9:
+                return True, flip_direction
+    else:
+        if deadtime <= deadtime_threshold:
+            return True, flip_direction
+    return False, flip_direction


Should: This function will benefit from a docstring to explain parameters, what it's doing etc.

DominicOram · 2023-06-29T10:54:17Z

+    direction = True
+    LOGGER.info(f"Target deadtime is {deadtime_threshold}")
+
+    while True:


Should: while True can accidentally end up infinite quickly. Can we have a max_cycles like for the other optimiser

DominicOram · 2023-06-29T10:58:21Z

+def deadtime_optimisation(
+    attenuator, xspress3mini, zebra, transmission, increment, deadtime_threshold
+):
+    direction = True


Nit: It would be more readable to have this set as an enum for positive/negative rather than True/False

DominicOram · 2023-06-29T10:59:31Z

+        total_time = xspress3mini.channel_1.total_time.get()
+        reset_ticks = xspress3mini.channel_1.reset_ticks.get()
+
+        LOGGER.info(f"Current total time = {total_time}")
+        LOGGER.info(f"Current reset ticks = {reset_ticks}")
+        deadtime = 0
+        if total_time != reset_ticks:
+            deadtime = 1 - abs(total_time - reset_ticks) / (total_time)


Should: Can we document what this calculation is doing and why?

e.g. what does reset_ticks mean?

DominicOram · 2023-06-29T11:01:46Z

+def deadtime_optimisation(
+    attenuator, xspress3mini, zebra, transmission, increment, deadtime_threshold
+):


Should: Can we have a docstring on this function and type hints?

I'll add a docstring to the total counts version as well

DominicOram · 2023-06-29T11:05:32Z

+        if transmission > 0.999:
+            transmission = 1


Should: I think @neil-i03 was talking about wanting a max value on transmission that we can set in a config somewhere. 100% is too high. Can you pull out a max/min into constants for now and have a separate function that checks them and throws errors if they're reached

DominicOram · 2023-06-29T13:52:08Z

+            # The 0.9 is hardcoded in GDA
+            if transmission >= 0.9:
+                return True, flip_direction


Must: Are we sure this is right? It looks wrong to me because if the deadtime is less than threshold and the transmission less than 90% the function returns nothing

I think this is the 90% transmission limit in GDA Neil was talking about. If the transmission is >=90% and deadtime is still higher than the threshold, it just uses that as the optimised transmission. I'll add parameters for max and min as you suggested in previous comment. Also I'll add a logging message if it uses the maximum transmission as the optimised value.

DominicOram · 2023-06-29T13:55:08Z

    yield from arm_zebra(zebra)


+def deadtime_is_transmission_optimised(


Could: It might be cleaner if the function returned whether you're optimised and then which direction to move in, rather than the flip

Could: It would be even more clear if we just separate it out into two functions, one to work out if we're there and one to work out if to flip. It might lead to some repeated work but clarity is more important than efficiency here

Should: Can we have some separate tests on this function

DominicOram · 2023-06-29T13:57:10Z

+            break
+
+        if flip_direction:
+            direction = not direction


Should: We need some tests that cause the direction to be flipped

DominicOram · 2023-06-29T13:58:47Z

+def test_deadtime_calc_new_transmission_gets_correct_value():
+    assert deadtime_calc_new_transmission(True, 0.05, 2) == 0.1
+    assert deadtime_calc_new_transmission(False, 0.05, 2) == 0.025
+    assert deadtime_calc_new_transmission(True, 1, 2) == 1


Nit: Better done using parameterize, will be clearer when one fails

olliesilvester · 2023-06-30T14:51:04Z

    return status


+@pytest.mark.skip(reason="Flakey test which is refactored in another PR")


Skipped this test for now as it's refactored in #763 (although still isn't working properly after that)

Coverage is low due to this test being skipped, but #763 should fix that

olliesilvester · 2023-07-03T15:11:49Z

+        deadtime = 0
+
+        """ Deadtime is the time after each event during which the detector cannot record another event.
+            The reset ticks PV stops ticking while the detector is unable to process events, so the absolute difference between the total time and the


I'm not 100% sure about this description of the reset ticks, but given the formula and definition for deadtime it looks like it must be something like this

I don't think we should have comments in that we're not sure about. Let's look more inhttps://github.com/DiamondLightSource/python-artemis/issues/776

DominicOram

One minor issue

DominicOram · 2023-07-04T15:42:50Z

+def get_new_direction(direction: Direction, deadtime, deadtime_threshold):
+    if direction == Direction.POSITIVE:
+        if deadtime >= deadtime_threshold:
+            direction = Direction.NEGATIVE
+    return direction
+
+
+def calculate_new_direction(direction: Direction, deadtime, deadtime_threshold):
+    if direction == Direction.POSITIVE:
+        if deadtime > deadtime_threshold:
+            direction = Direction.NEGATIVE
+    return direction


Should: These functions are basically the same, only one is used, can we remove the other?

olliesilvester commented Jun 23, 2023

View reviewed changes

olliesilvester force-pushed the 754_add_deadtime_optimisation branch 2 times, most recently from 25f8d94 to 4bf3e1f Compare June 28, 2023 09:53

olliesilvester marked this pull request as ready for review June 28, 2023 12:07

olliesilvester requested a review from DominicOram June 28, 2023 12:07

This was referenced Jun 28, 2023

Artemis 754 add deadtime optimisation DiamondLightSource/dodal#102

Merged

760 refactor total count optimisation tests #763

Closed

DominicOram suggested changes Jun 29, 2023

View reviewed changes

olliesilvester commented Jun 30, 2023

View reviewed changes

olliesilvester requested a review from DominicOram June 30, 2023 15:36

olliesilvester added 19 commits July 3, 2023 15:44

added deadtime optimisation function

78bfce9

add is_deadtime_optimised, remove read_scalar values

d959b6f

add calc_new_transmission

0328057

add do_device_optimise_iteration

da84647

minor restructure

2cc800b

Add overall test for deadtime optimise

8682b2b

initialise deadtime to 0

a379f2a

update dodal url

12acb95

set optimisation type as plan parameter instead

fe45998

remove hardcoded optimisation type from plan

9ad51c8

more unit tests

154613c

fix arming devices

f792b22

dodal url

c069735

Added comments and made direction an Enum

c0ddb7a

corrected comments

9d73376

Minor tidy up and further documenting

5ad5e3a

Refactor of deadtime optimisation

a338a14

Decompose unit tests

92625d9

dodal url

b9acee6

olliesilvester added 2 commits July 3, 2023 15:47

fix tests

4d8e756

update dodal url

76c9209

olliesilvester force-pushed the 754_add_deadtime_optimisation branch from d85f7e1 to 76c9209 Compare July 3, 2023 14:47

olliesilvester commented Jul 3, 2023

View reviewed changes

adjust comments and wait for optimised transmission

bd024a0

DominicOram approved these changes Jul 4, 2023

View reviewed changes

DominicOram added 3 commits July 4, 2023 16:54

Merge branch 'main' into 754_add_deadtime_optimisation

4a1e7c6

(#754) Fix review comment

a3a7d5b

(#754) Fix tests

93e800e

DominicOram merged commit 6b7abf5 into main Jul 4, 2023

DominicOram deleted the 754_add_deadtime_optimisation branch July 4, 2023 16:28

		yield from arm_zebra(zebra)


		def deadtime_is_transmission_optimised(

		return status


		@pytest.mark.skip(reason="Flakey test which is refactored in another PR")

Uh oh!

Conversation

olliesilvester commented Jun 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

To test:

Uh oh!

codecov Bot commented Jun 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DominicOram left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

olliesilvester commented Jun 22, 2023 •

edited

Loading

codecov Bot commented Jun 22, 2023 •

edited

Loading