Skip to content
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.

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

use devicevector #799

Closed
wants to merge 4 commits into from
Closed

Conversation

stan-dot
Copy link
Contributor

@stan-dot stan-dot commented Sep 23, 2024

Fixes #795

Instructions to reviewer on how to test:

  1. Do dodal connect i22
  2. Confirm connection happens
  3. Do dodal connect p38
  4. Confirm sim mode connection happens

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}

@stan-dot stan-dot added the enhancement New feature or request label Sep 23, 2024
@stan-dot stan-dot self-assigned this Sep 23, 2024
@stan-dot stan-dot linked an issue Sep 23, 2024 that may be closed by this pull request
Copy link

codecov bot commented Sep 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.97%. Comparing base (eece064) to head (18dbbeb).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #799      +/-   ##
==========================================
- Coverage   94.99%   94.97%   -0.03%     
==========================================
  Files         115      115              
  Lines        4719     4698      -21     
==========================================
- Hits         4483     4462      -21     
  Misses        236      236              

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

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.

I think this is a worse interface for the users. The grouping of devices in the beamline file should be logical to a scientist as that is their interface with the devices. They do not think of the slits on the beamline as one large device with multiple slits but rather as individual devices. So they would want to define a plan like def slit_scan(slit: Slits = inject("slits_1")). This also brings the issue of how we deal with disconnected slits. It is very possible that the beamline may have one slit set offline but still want to use the others. With SixSlits the whole device will fail to connect and throw errors.

@stan-dot
Copy link
Contributor Author

@DominicOram at the moment I only see the Slits device used at i22 and p38, where I know the scientists and their ways of working and for now they don't write tests themselves. And the device is just in the devices/i22 folder and any beamline can use the single Slits device however they want.

With SixSlits the whole device will fail to connect and throw errors.

that is a valid point, might need a decorator 'allow fail'? I am not sure if the slits are used independently like what you're describing now at i22

@DiamondJoseph
Copy link
Contributor

If we go this way I don't think it should be called six_slits: we'd still be able to address individual slits with e.g. inject by doing inject = ("slits.1") (NB: Might not work for device vectors? Would this work with inject("slits[1]")?)

@stan-dot
Copy link
Contributor Author

at the latest version it's a SlitsCollection. testing out ipython behavior of device factor shortly

@stan-dot
Copy link
Contributor Author

@DominicOram what is a plan that you'd like to define in ipython and then run it that the inject behavior would be used?

@DominicOram
Copy link
Contributor

@DominicOram what is a plan that you'd like to define in ipython and then run it that the inject behaviour would be used?

Something like:

def slit_scan(slit: Slits = inject("slits_1"), i0: TetrammDetector = inject("i0"):
    yield from bps.scan([i0], slit.x, 0, 100)

@stan-dot
Copy link
Contributor Author

ok how do I get the beamline module to be referred to with inject?

I don't see this in tutorials

search also does not provide information on the use of dodal in ipython https://diamondlightsource.github.io/dodal/main/search.html?q=ipython

@DiamondJoseph
Copy link
Contributor

DiamondJoseph commented Sep 29, 2024

ok how do I get the beamline module to be referred to with inject?

It's a blueapi-ism, assuming that there is a device with that name in the blueapi context that will fetchable when the pydantic model is deserialised: regardless of what beamline module is loaded. It's not particularly well developed or documented, I'm hoping there's something in Pydantic 2 that we'll be able to leverage for the same effect but I will have to look after I get back.

@stan-dot
Copy link
Contributor Author

@DominicOram on re-reading your initial comment I see that the granularity of skip_device decorator is decreased as a result of this PR, closing the PR then

@stan-dot stan-dot closed this Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make the slits use DeviceVector
3 participants