Skip to content

Add the diamond filter and qbpm devices#820

Merged
DominicOram merged 4 commits intomainfrom
mx_bluesky_544_scan_diamond_filter
Oct 14, 2024
Merged

Add the diamond filter and qbpm devices#820
DominicOram merged 4 commits intomainfrom
mx_bluesky_544_scan_diamond_filter

Conversation

@DominicOram
Copy link
Contributor

Part of DiamondLightSource/mx-bluesky#544

Instructions to reviewer on how to test:

  1. Run dodal connect i03 and dodal connect i04 and confirm they 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}

@codecov
Copy link

codecov bot commented Oct 8, 2024

Codecov Report

Attention: Patch coverage is 94.87179% with 2 lines in your changes missing coverage. Please review.

Project coverage is 95.03%. Comparing base (507f9ea) to head (cf26232).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/dodal/beamlines/i03.py 83.33% 1 Missing ⚠️
src/dodal/beamlines/i04.py 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #820      +/-   ##
==========================================
- Coverage   95.03%   95.03%   -0.01%     
==========================================
  Files         117      119       +2     
  Lines        4771     4810      +39     
==========================================
+ Hits         4534     4571      +37     
- Misses        237      239       +2     

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

T = TypeVar("T", bound=Filters)


class DiamondFilter(StandardReadable, Generic[T]):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@coretl or @callumforrester. Do we like this pattern for the different enums on each beamline or do you think better to just drop it and leave it as a str?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Depends, do you in code write filter.thickness.set(I03Filters.FIFTY) anywhere? If so then it's useful to check that the specified filter value exists at connect too. If not then drop to string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I understand. We don't write filter.thickness.set(I03Filters.FIFTY) anywhere because there is no 50um filter on I03. The filters are different on each beamline but otherwise the hardware is the same. What I'm trying to do is allow people to write:

i03_filter = i03.filter()
i04_filter = i04.filter()

i03_filter.thickness.set(I03Filters.TWO_HUNDRED)  # All good
i03_filter.thickness.set(I04Filters.FIFTY) # Type system gives an error here, there is no 50 on i03

My concern with str was that you can then do:

i03_filter.thickness.set("200 um")  # All good
i03_filter.thickness.set("50 um") # No warning this is not valid until runtime

I wanted to make sure this pattern wasn't overly complicating things or doing something unexpected for ophyd_async though

Copy link
Collaborator

Choose a reason for hiding this comment

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

The procedural case will remain compatible with this, but making the declarative approach work for this would be overkill. I'd say it makes sense for the usecase you mention.

Copy link
Contributor

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

Looks good, and devices behaved on i03. I think what you've done with generics is good too

Comment on lines +43 to +44
self.y_motor = Motor(prefix + "Y")
self.thickness = epics_signal_rw(data_type, f"{prefix}Y:MP:SELECT")
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: It's not immediately obvious (to me) that selecting a thickness is just selecting a motor value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a bit more in the docstring

@DominicOram DominicOram merged commit 4cc0811 into main Oct 14, 2024
@DominicOram DominicOram deleted the mx_bluesky_544_scan_diamond_filter branch October 14, 2024 13:35
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.

3 participants