Skip to content

Extend monochrometer to get energy from y position#2072

Merged
DominicOram merged 11 commits into
mainfrom
cryst_bluesky_47_read_energy
May 28, 2026
Merged

Extend monochrometer to get energy from y position#2072
DominicOram merged 11 commits into
mainfrom
cryst_bluesky_47_read_energy

Conversation

@jacob720
Copy link
Copy Markdown
Contributor

@jacob720 jacob720 commented May 26, 2026

Fixes DiamondLightSource/crystallography-bluesky#47

Requires DiamondLightSource/daq-config-server#183

Instructions to reviewer on how to test:

  1. Check tests pass

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}

@jacob720 jacob720 requested a review from a team as a code owner May 26, 2026 09:39
Copy link
Copy Markdown
Contributor

@DominicOram DominicOram left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks. Could you pin to the correct version of the daq config server so that the tests pass, with a comment saying to remove the pin once there's a release?

return self.config_client.get_file_contents(
self.crystal_lut_path,
XpdfCrystalLookupTable,
# Remove once new config server is released + deployed
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should: Can you link to which PR the release needs for this

self.roll = Motor(prefix + "ROLL")
self.yaw = Motor(prefix + "YAW")
self.y = Motor(prefix + "Y")
self.energy_kev = derived_signal_r(self._get_energy, y=self.y)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should: I think this needs to be a hinted signal with the others being config

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the other signals being config? Or _config_client and _crystal_lut_path (which are now not signals)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what you've got is correct. Basically the one signal that is the most scientifically relevant should be hinted, everything else config

Comment on lines +22 to +23
self.config_client = config_client
self.crystal_lut_path = crystal_lut_path
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should: These aren't signals so don't need to be added as readables

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, they should probably be private

laue_monochrometer: LaueMonochrometer, y, expected_energy
):
await laue_monochrometer.y.set(y)
assert await laue_monochrometer.energy_kev.get_value() == expected_energy
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could: Can you test that read in here also gives the energy? Should be able to use assert_reading

@jacob720 jacob720 requested a review from DominicOram May 26, 2026 15:04
@codecov
Copy link
Copy Markdown

codecov Bot commented May 26, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.14%. Comparing base (bde3b69) to head (f2d8100).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2072   +/-   ##
=======================================
  Coverage   99.14%   99.14%           
=======================================
  Files         341      341           
  Lines       13275    13287   +12     
=======================================
+ Hits        13161    13173   +12     
  Misses        114      114           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Contributor

@DominicOram DominicOram left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thank you!

@DominicOram DominicOram enabled auto-merge (squash) May 28, 2026 16:41
@DominicOram DominicOram merged commit db3289e into main May 28, 2026
10 checks passed
@DominicOram DominicOram deleted the cryst_bluesky_47_read_energy branch May 28, 2026 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

i15-1: Allow reading energy

2 participants