Skip to content

Removes/reduces referenece use on i10 IDs #1032

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 63 commits into from
Jun 11, 2025
Merged

Removes/reduces referenece use on i10 IDs #1032

merged 63 commits into from
Jun 11, 2025

Conversation

Relm-Arrowny
Copy link
Contributor

@Relm-Arrowny Relm-Arrowny commented Jan 29, 2025 •
edited
Loading

Fixes #1029 and #1046.

Instructions to reviewer on how to test:

  1. Run test.
  2. Confirm connection.
  3. Check uml.

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}

@Relm-Arrowny Relm-Arrowny linked an issue Jan 29, 2025 that may be closed by this pull request
Removes/reduces refenece use on i10 IDs #1029
Closed
9 tasks
@codecov Codecov
Copy link

codecov bot commented Jan 31, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.04%. Comparing base (4aa9d22) to head (3c83944).
Report is 1 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1032      +/-   ##
==========================================
- Coverage   98.06%   98.04%   -0.02%     
==========================================
  Files         205      205              
  Lines        7802     7792      -10     
==========================================
- Hits         7651     7640      -11     
- Misses        151      152       +1

☔ 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.

@Relm-Arrowny Relm-Arrowny linked an issue Feb 7, 2025 that may be closed by this pull request
PGM/appl2_undulator timing out early #1046
Closed
1 task
@Relm-Arrowny Relm-Arrowny mentioned this pull request Feb 10, 2025
Merged
4 tasks
@Relm-Arrowny Relm-Arrowny marked this pull request as ready for review February 11, 2025 09:35
@Relm-Arrowny Relm-Arrowny requested a review from a team as a code owner February 11, 2025 09:35
@DominicOram DominicOram changed the title Removes/reduces refenece use on i10 IDs Removes/reduces referenece use on i10 IDs Feb 12, 2025
DominicOram
DominicOram requested changes Feb 24, 2025
View reviewed changes
Copy link
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.

Thanks, I think it's cleaner but have some more comments to make it even more so.

@DominicOram DominicOram mentioned this pull request Feb 27, 2025
Closed
@Relm-Arrowny Relm-Arrowny mentioned this pull request Jun 4, 2025
Open
3 tasks
olliesilvester
olliesilvester reviewed Jun 4, 2025
View reviewed changes
src/dodal/devices/apple2_undulator.py
"""This change the position of all the motors for a given energy and
polarisation_setpoint"""

def _read_pol(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand the device well enough to comment on the automatic switch, but otherwise that sounds good I think, as long as users don't get confused when their readback says LH instead of LH3

olliesilvester
olliesilvester requested changes Jun 9, 2025
View reviewed changes
Copy link
Collaborator

@olliesilvester olliesilvester left a comment

Choose a reason for hiding this comment

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

Sorry, just a few more comments. I think it's very almost there now

olliesilvester
olliesilvester approved these changes Jun 10, 2025
View reviewed changes
Copy link
Collaborator

@olliesilvester olliesilvester left a comment

Choose a reason for hiding this comment

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

It's possible I've missed something as I don't have fresh eyes on this code anymore, but I think it looks good now, thanks!

I have a nit on the tests, but I suggest you don't do them in this PR so we can get this one merged: IMO it's better to test the features of your base class in the test file of that base class, rather than the test file of an implementation of that class. For example, Apple2.determine_phase_from_hardware is tested in test_i10Apple2.py rather than test_apple2_undulator.py

@olliesilvester olliesilvester dismissed DominicOram’s stale review June 11, 2025 10:29

Changes have been addressed

@Relm-Arrowny Relm-Arrowny merged commit b7c9c51 into main Jun 11, 2025
19 checks passed
@Relm-Arrowny Relm-Arrowny deleted the 1029-removesreduces-refenece-use-on-i10-ids branch June 11, 2025 11:25
olliesilvester added a commit that referenced this pull request Jun 12, 2025
@Relm-Arrowny @DominicOram @olliesilvester
Removes/reduces referenece use on i10 IDs (#1032)
92d0627 
* corrected read so it always read pol from hardware

* add uml and correct some docstring

* Time out fix
* change id to use direved derived_signal_r

* make polarisation both read and set.

* update lookuptable path for blueapi

---------

Co-authored-by: Dominic Oram <[email protected]>
Co-authored-by: olliesilvester <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olliesilvester olliesilvester olliesilvester approved these changes

@DominicOram DominicOram Awaiting requested review from DominicOram

Assignees

@Relm-Arrowny Relm-Arrowny

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

PGM/appl2_undulator timing out early Removes/reduces refenece use on i10 IDs
3 participants
@Relm-Arrowny @DominicOram @olliesilvester