Crystal metadata change #843

stan-dot · 2024-10-16T11:33:02Z

Code extracted from https://github.com/DiamondLightSource/dodal/pull/722/files

I don't quite understand why do we set this as a soft signal - that is one failing test at the moment

        # Soft metadata
        # If supplied include crystal details in output of read_configuration
        with self.add_children_as_readables(ConfigSignal):
            if crystal_1_metadata.usage is not None:
                self.crystal_1_usage, _ = soft_signal_r_and_setter(
                    str, initial_value=crystal_1_metadata.usage
                )
            else:
                self.crystal_1_usage = None

And I deleted one test too

stan-dot · 2024-10-21T10:53:07Z

the error has to do with crystal spacing values

callumforrester · 2024-10-21T11:04:07Z

What is the reason for this change?

stan-dot · 2024-10-21T12:51:48Z

@callumforrester this is a refactor as d_spacing can be directly traced from the reflection values and the material properties.

Neater for the i18 lookup service

src/dodal/common/crystal_metadata.py

rtuck99 · 2024-10-25T13:12:05Z

@stan-dot I have raised a PR #870 which targets this branch and extends these changes onto the I03 DCM, as a prerequisite for DiamondLightSource/mx-bluesky#520

~~@JamesOHeaDLS do you know if the I03 DCM also supports getting the d-spacing from epics?~~

stan-dot · 2024-10-28T11:01:08Z

thanks @rtuck99 , merged

DominicOram

Thanks Stan. Looks like i18 has the PVs now. Some comments in code though

src/dodal/devices/dcm.py

src/dodal/common/crystal_metadata.py

stan-dot · 2024-10-29T11:33:14Z

<<<<<<< HEAD
async def test_crystal_metadata_not_propagated_when_not_supplied():
    async with DeviceCollector(mock=True):
        dcm = DoubleCrystalMonochromator(
            prefix="FOO-MO",
            temperature_prefix="FOO-DI",
            crystal_1_metadata=None,
            crystal_2_metadata=None,
        )

    configuration = await dcm.read_configuration()
    expected_absent_keys = {
        "crystal-1-usage",
        "crystal-1-type",
        "crystal-1-reflection",
        "crystal-1-d_spacing",
        "crystal-2-usage",
        "crystal-2-type",
        "crystal-2-reflection",
        "crystal-2-d_spacing",
    }
    assert expected_absent_keys.isdisjoint(configuration)


=======
>>>>>>> e8ac54553 (add to the beamline files)

@DominicOram all is fixed besides this test. I am not sure what is the purpose of the behavior here to have absent keys. For it to pass the metadata param in the DCM init would need to be CrystalMetadata | None and not CrystalMetadata.

I'm happy to delete the test, git blame points last modification to the test, for May 2024 by @callumforrester - what is the reason for the test? I'm equally happy to keep the test and change the behavior provided I note the reason for allowing None in the code.

DominicOram · 2024-10-29T13:25:19Z

all is fixed besides this test...

Ok, I'm happy to retract https://github.com/DiamondLightSource/dodal/pull/843/files#r1820621900 so that we can keep this usecase. I'm still not 100% sure when it would be necessary but that's scope creep.

stan-dot · 2024-10-30T17:22:08Z

@DominicOram I'm afraid I don't understand your last comment

codecov · 2024-11-04T12:26:19Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.61%. Comparing base (2e58d4e) to head (d3e79cd).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #843      +/-   ##
==========================================
+ Coverage   95.60%   95.61%   +0.01%     
==========================================
  Files         125      126       +1     
  Lines        5229     5242      +13     
==========================================
+ Hits         4999     5012      +13     
  Misses        230      230

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

Previously this was hard coded to be a static float in milimeters for some reason, forgot to update this for this PR

* Implement crystal metadata for I03 DCM * Make type-checking happy

stan-dot · 2024-11-05T14:28:21Z

 self.crystal_metadata_reflection, _ = soft_signal_r_and_setter(
                Sequence[int],
                initial_value=list(cm.reflection),  # type: ignore
            )

I think this might be due to ophyd-async change - this is in the 'dcm' device

DominicOram

Thanks @stan-dot. Couple of minor queries, I think I may have misunderstood a previous discussion.

DominicOram · 2024-11-05T19:42:11Z

src/dodal/common/crystal_metadata.py

+    d_spacing: tuple[float, str]
+
+    @staticmethod
+    def calculate_default_d_spacing(


Should: Sorry, maybe I've misunderstood, I thought the conclusion of the discussion at #843 (comment) was to remove all this calculation and use the PV? Or is it not yet on i22?

If the PV isn't on i22 yet then I said in that comment, can we get the ball rolling on that?

there was a discussion was to add the PV on i18, which ultimately just was a slight name change.

regarding the static method

self.crystal_metadata_d_spacing = epics_signal_r(float, "DSPACING:RBV")

at the moment there is the PV value used. I might have mistaken with the line above where the 'initial value' is used.

at the moment the only place when the calculation is used is in the make_crystal_metadata_from_material where if the d_spacing is not supplied by the user, it's calculated.

we could remove it and make it required

Ok, so we still need it in there for i22. That's fine, I'll make a note on the conversion issue.

DominicOram · 2024-11-05T19:44:40Z

src/dodal/devices/i22/dcm.py

@@ -74,63 +61,45 @@ def __init__(

        # Soft metadata
        # If supplied include crystal details in output of read_configuration
-        crystal_1_metadata = crystal_1_metadata or CrystalMetadata()
-        crystal_2_metadata = crystal_2_metadata or CrystalMetadata()
        with self.add_children_as_readables(ConfigSignal):
            if crystal_1_metadata.usage is not None:


Should: If we've now removed the option for these to be None then I don't think we need to check them all?

DominicOram · 2024-11-05T19:46:03Z

tests/devices/unit_tests/test_dcm.py

+async def test_metadata_reflection(dcm: DCM):
+    signal = dcm.crystal_metadata_reflection
+    v = await signal.read()
+    print(v)


Should: It's not best practice to leave print statements in code

DominicOram

Looks good to me, thank you!

stan-dot added enhancement New feature or request i18 i22 labels Oct 16, 2024

stan-dot requested review from DominicOram and callumforrester October 16, 2024 11:33

stan-dot self-assigned this Oct 16, 2024

stan-dot mentioned this pull request Oct 16, 2024

add i18 beamline definition - WITH device_factory #722

Open

4 tasks

rtuck99 mentioned this pull request Oct 18, 2024

Energy Changes: Calculate the bragg angle ourselves DiamondLightSource/mx-bluesky#520

Closed

stan-dot marked this pull request as ready for review October 21, 2024 11:15

rtuck99 reviewed Oct 21, 2024

View reviewed changes

src/dodal/common/crystal_metadata.py Show resolved Hide resolved

stan-dot requested a review from rtuck99 October 28, 2024 12:09

DominicOram requested changes Oct 29, 2024

View reviewed changes

src/dodal/devices/dcm.py Outdated Show resolved Hide resolved

src/dodal/common/crystal_metadata.py Outdated Show resolved Hide resolved

src/dodal/common/crystal_metadata.py Outdated Show resolved Hide resolved

src/dodal/common/crystal_metadata.py Outdated Show resolved Hide resolved

stan-dot force-pushed the crystal-metadata branch 2 times, most recently from c46f234 to 6ee887d Compare November 4, 2024 12:21

stan-dot force-pushed the crystal-metadata branch from 377c46c to 999c1b0 Compare November 4, 2024 14:21

stan-dot and others added 6 commits November 5, 2024 14:16

add the class and a test

9592db2

add to the beamline files

f707b51

Fix the expected test spacing values

103e177

Previously this was hard coded to be a static float in milimeters for some reason, forgot to update this for this PR

Implement crystal metadata for I03 DCM (#870)

0aee3ea

* Implement crystal metadata for I03 DCM * Make type-checking happy

make d spacing a signal

11871a6

delete the absent keys test

edd9fef

stan-dot added 3 commits November 5, 2024 14:17

remove the d spacing calculation

f2e8920

respond to feedback

060784d

fix crystal metadata variable namign

b489f57

stan-dot force-pushed the crystal-metadata branch from 854a2c5 to b489f57 Compare November 5, 2024 14:18

stan-dot added 3 commits November 5, 2024 14:49

convert the types correctly

be6aee6

ruff

961aa70

apparently this fixes tests and coverage

749bf8a

stan-dot requested a review from DominicOram November 5, 2024 15:23

DominicOram requested changes Nov 5, 2024

View reviewed changes

stan-dot added 2 commits November 6, 2024 09:22

remove print

cb9abcf

remove is None checks

d3e79cd

DominicOram approved these changes Nov 6, 2024

View reviewed changes

stan-dot merged commit bcd40a3 into main Nov 6, 2024
19 checks passed

stan-dot deleted the crystal-metadata branch November 6, 2024 10:29

stan-dot mentioned this pull request Nov 25, 2024

Add a CrystalMetadata enum #837

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Crystal metadata change #843

Crystal metadata change #843

stan-dot commented Oct 16, 2024

stan-dot commented Oct 21, 2024

callumforrester commented Oct 21, 2024

stan-dot commented Oct 21, 2024

rtuck99 commented Oct 25, 2024 •

edited

Loading

stan-dot commented Oct 28, 2024

DominicOram left a comment

stan-dot commented Oct 29, 2024

DominicOram commented Oct 29, 2024

stan-dot commented Oct 30, 2024

codecov bot commented Nov 4, 2024 •

edited

Loading

stan-dot commented Nov 5, 2024 •

edited

Loading

DominicOram left a comment

DominicOram Nov 5, 2024

DominicOram Nov 5, 2024

stan-dot Nov 6, 2024

DominicOram Nov 6, 2024

DominicOram Nov 5, 2024

stan-dot Nov 6, 2024

DominicOram Nov 5, 2024

DominicOram left a comment

Crystal metadata change #843

Crystal metadata change #843

Conversation

stan-dot commented Oct 16, 2024

stan-dot commented Oct 21, 2024

callumforrester commented Oct 21, 2024

stan-dot commented Oct 21, 2024

rtuck99 commented Oct 25, 2024 • edited Loading

stan-dot commented Oct 28, 2024

DominicOram left a comment

Choose a reason for hiding this comment

stan-dot commented Oct 29, 2024

DominicOram commented Oct 29, 2024

stan-dot commented Oct 30, 2024

codecov bot commented Nov 4, 2024 • edited Loading

Codecov Report

stan-dot commented Nov 5, 2024 • edited Loading

DominicOram left a comment

Choose a reason for hiding this comment

DominicOram Nov 5, 2024

Choose a reason for hiding this comment

DominicOram Nov 5, 2024

Choose a reason for hiding this comment

stan-dot Nov 6, 2024

Choose a reason for hiding this comment

DominicOram Nov 6, 2024

Choose a reason for hiding this comment

DominicOram Nov 5, 2024

Choose a reason for hiding this comment

stan-dot Nov 6, 2024

Choose a reason for hiding this comment

DominicOram Nov 5, 2024

Choose a reason for hiding this comment

DominicOram left a comment

Choose a reason for hiding this comment

rtuck99 commented Oct 25, 2024 •

edited

Loading

codecov bot commented Nov 4, 2024 •

edited

Loading

stan-dot commented Nov 5, 2024 •

edited

Loading