redo the i18 add #1030

stan-dot · 2025-01-28T13:31:29Z

Fixes #709

Instructions to reviewer on how to test:

Run Dodal connect i18
Confirm devices connect

Checks for reviewer

Would the PR title make sense to a scientist on a set of release notes
If a new device has been added does it follow the standards
If changing the API for a pre-existing device, ensure that any beamlines using this device have updated their Bluesky plans accordingly
Have the connection tests for the relevant beamline(s) been run via dodal connect ${BEAMLINE}

codecov · 2025-01-28T13:56:25Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.63%. Comparing base (84e49b5) to head (718ae2d).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1030      +/-   ##
==========================================
+ Coverage   97.60%   97.63%   +0.03%     
==========================================
  Files         154      159       +5     
  Lines        6468     6564      +96     
==========================================
+ Hits         6313     6409      +96     
  Misses        155      155

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tests/devices/i18/test_thor_labs_stage.py

tests/devices/i18/test_table.py

DiamondJoseph · 2025-01-28T13:39:01Z

tests/devices/i18/test_table.py

+    set_mock_value(table.x.user_readback, 1.23)
+    set_mock_value(table.y.user_readback, 4.56)
+


must

Suggested change

set_mock_value(table.x.user_readback, 1.23)

set_mock_value(table.y.user_readback, 4.56)

DiamondJoseph · 2025-01-28T13:39:34Z

tests/devices/i18/test_kb_mirror.py

+    # Mock the initial values of the x and y signals
+    set_mock_value(kbmirror.x, 0.0)
+    set_mock_value(kbmirror.y, 0.0)


Should: 0 is the default value for a float signal, so this shouldn't be required.

Suggested change

# Mock the initial values of the x and y signals

set_mock_value(kbmirror.x, 0.0)

set_mock_value(kbmirror.y, 0.0)

DiamondJoseph · 2025-01-28T13:42:30Z

tests/devices/i18/test_table.py

+            "alarm_severity": 0,
+            "timestamp": ANY,
+            "value": 0.0,
+        },
+        "table-z": {"alarm_severity": 0, "timestamp": ANY, "value": 0.0},
+    }


Suggested change

"alarm_severity": 0,

"timestamp": ANY,

"value": 0.0,

},

"table-z": {"alarm_severity": 0, "timestamp": ANY, "value": 0.0},

}

"value": 0.0,

"timestamp": ANY,

"alarm_severity": 0,

},

"table-z": {

"value": 0.0,

"timestamp": ANY,

"alarm_severity": 0,

},

}

nit: consistency with above

DiamondJoseph · 2025-01-28T13:43:12Z

tests/devices/i18/test_table.py

+        "table-theta": {
+            "alarm_severity": 0,
+            "timestamp": ANY,
+            "value": 10.11,
+        },
+        "table-z": {"alarm_severity": 0, "timestamp": ANY, "value": 7.89},


nit: make this consistent.

what do you mean?

Make the formatting of "table-z" the same as the formatting of "table-theta"

stan-dot · 2025-01-28T15:18:17Z

it would seem that the set_mock_value is indeed required, the tests are failing now

https://github.com/DiamondLightSource/dodal/actions/runs/13013408223/job/36296490106?pr=1030

DiamondJoseph · 2025-01-28T15:33:50Z

it would seem that the set_mock_value is indeed required, the tests are failing now

dodal/tests/devices/unit_tests/test_shutter.py

Line 20 in 5c82bbd

def propagate_status(value: ZebraShutterState, *args, **kwargs):

You want something like this callback_on_mock_put that ensure the _RBV signal gets updated when you set the write half.

stan-dot · 2025-01-28T15:43:19Z

    def propagate_status(value: TablePosition, *args, **kwargs):
        set_mock_value(table.x.user_readback, value.x)
        set_mock_value(table.y.user_readback, value.y)
        set_mock_value(table.z.user_readback, value.z)
        set_mock_value(table.theta.user_readback, value.theta)

    callback_on_mock_put(table, propagate_status)
    return table

okay but its per-signal basis so do I need to make 4 of those? set_mock_value is quite low-granular

DiamondJoseph · 2025-01-28T16:00:02Z

okay but its per-signal basis so do I need to make 4 of those? set_mock_value is quite low-granular

#1031
Might be worth getting this nailed down.

stan-dot · 2025-01-28T16:07:28Z

Might be worth getting this nailed down.

what do you mean by that?

callumforrester · 2025-02-03T12:45:53Z

@stan-dot resolving #1031 would dictate whether or not we keep the set methods you've added to your devices. As far as I can see they are there to make setting the positions of a group of sub-motors into an atomic operation.

This PR is already long-standing and I do not think we need to resolve #1031 to everyone's satisfaction in order to get it merged. The discussion in there is trending towards a consensus:

set is most useful if it has an obvious meaning, where a device is associated with a singular, scalar value (e.g. a motor).
It is sometimes useful to streamline an operation that a user does a lot
Occasionally there are specific requirements for making "setting" a device into an atomic operation.
This is also roughly consistent with how dodal is today, or at least its direction of travel.

As far as I can see there's no concrete use case for atomicity for any of these devices, I don't think that it streamlines the operation from the user point of view since

yield from mv(mirror, XYPosition(1.0, 2.0))

Is the same as

yield from mv(mirror.x, 1.0, mirror.y, 2.0)

The other possibility is that you think the first example is neater/more readable. I won't comment on that except to say that it's usually more important to be consistent with the rest of the codebase than to write exactly what makes the most sense to you, since a future developer will read a broad swathe of code in dodal and will find it easiest to read consistent code.

So in summary I think the quickest path to getting this merged is either removing the set methods or justifying them against the criteria above.

callumforrester

LGTM :)

.

stan-dot added enhancement New feature or request i18 labels Jan 28, 2025

stan-dot self-assigned this Jan 28, 2025

stan-dot requested a review from a team as a code owner January 28, 2025 13:31

DiamondJoseph self-assigned this Jan 28, 2025

stan-dot mentioned this pull request Jan 28, 2025

add i18 beamline definition - WITH device_factory #722

Closed

4 tasks

stan-dot requested a review from DiamondJoseph January 28, 2025 13:54

DiamondJoseph previously requested changes Jan 28, 2025

View reviewed changes

DiamondJoseph removed their assignment Jan 28, 2025

stan-dot force-pushed the add-i18-bis branch 2 times, most recently from 0ab008e to 68ffb09 Compare January 30, 2025 10:30

stan-dot added 6 commits February 3, 2025 10:43

redo the i18 add

770a381

fix the thor labs test

d08c621

make tests set mockto 0

02507bc

respond partially to feedback

4c0723d

fix thor labs device name in test

e143603

leave a note on crystal metadata

ac41155

stan-dot force-pushed the add-i18-bis branch from 0ad043b to ac41155 Compare February 3, 2025 10:43

remove Movable interface on new devices

f71f358

stan-dot requested a review from callumforrester February 3, 2025 13:08

stan-dot added 2 commits February 3, 2025 13:09

fix typo

d207338

make tests use mock

718ae2d

callumforrester approved these changes Feb 3, 2025

View reviewed changes

stan-dot merged commit 8061351 into main Feb 3, 2025
19 checks passed

stan-dot deleted the add-i18-bis branch February 3, 2025 14:11

callumforrester mentioned this pull request Feb 3, 2025

Document when to make a Device class extend Movable #1031

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

redo the i18 add #1030

redo the i18 add #1030

stan-dot commented Jan 28, 2025

codecov bot commented Jan 28, 2025 •

edited

Loading

DiamondJoseph Jan 28, 2025

DiamondJoseph Jan 28, 2025

DiamondJoseph Jan 28, 2025

DiamondJoseph Jan 28, 2025

stan-dot Jan 28, 2025

callumforrester Feb 3, 2025

stan-dot commented Jan 28, 2025

DiamondJoseph commented Jan 28, 2025

stan-dot commented Jan 28, 2025

DiamondJoseph commented Jan 28, 2025

stan-dot commented Jan 28, 2025

callumforrester commented Feb 3, 2025

callumforrester left a comment

		set_mock_value(table.x.user_readback, 1.23)
		set_mock_value(table.y.user_readback, 4.56)

	# Mock the initial values of the x and y signals
	set_mock_value(kbmirror.x, 0.0)
	set_mock_value(kbmirror.y, 0.0)

redo the i18 add #1030

redo the i18 add #1030

Conversation

stan-dot commented Jan 28, 2025

Instructions to reviewer on how to test:

Checks for reviewer

codecov bot commented Jan 28, 2025 • edited Loading

Codecov Report

DiamondJoseph Jan 28, 2025

Choose a reason for hiding this comment

DiamondJoseph Jan 28, 2025

Choose a reason for hiding this comment

DiamondJoseph Jan 28, 2025

Choose a reason for hiding this comment

DiamondJoseph Jan 28, 2025

Choose a reason for hiding this comment

stan-dot Jan 28, 2025

Choose a reason for hiding this comment

callumforrester Feb 3, 2025

Choose a reason for hiding this comment

stan-dot commented Jan 28, 2025

DiamondJoseph commented Jan 28, 2025

stan-dot commented Jan 28, 2025

DiamondJoseph commented Jan 28, 2025

stan-dot commented Jan 28, 2025

callumforrester commented Feb 3, 2025

callumforrester left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 28, 2025 •

edited

Loading