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

601 convert smargon to ophyd async #656

Merged
merged 5 commits into from
Jul 8, 2024
Merged

Conversation

rtuck99
Copy link
Contributor

@rtuck99 rtuck99 commented Jul 2, 2024

Fixes #601

See also DiamondLightSource/hyperion#1469

Note that the I23 gonio, XYZLimitBundle and GridScanParams.is_valid() logic were all removed as unused.

Instructions to reviewer on how to test:

  1. Tests should pass
  2. diffraction experiments should continue to work

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}

@rtuck99 rtuck99 force-pushed the 601_convert_smargon_to_ophyd_async branch from cc16c11 to 097b332 Compare July 2, 2024 14:01
@rtuck99 rtuck99 requested review from d-perl and noemifrisina July 2, 2024 14:11
@rtuck99 rtuck99 marked this pull request as ready for review July 2, 2024 14:11
@rtuck99 rtuck99 requested a review from olliesilvester July 2, 2024 14:26
Copy link
Contributor

@d-perl d-perl 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, few nits, take em or leave em as you please

min_value: float
max_value: float

def contains(self, pos: float):
Copy link
Contributor

Choose a reason for hiding this comment

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

Was it a deliberate choice not to use __contains__ here?

@@ -1,10 +1,13 @@
from functools import partial
from typing import Callable

from bluesky.protocols import Movable
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I would like it better if we did something like

from ophyd_async.core import device as OphydAsyncDevice
from ophyd import device as OphydDevice

or alternatively to use module.object syntax for both ophyd and ophyd_async items

]
)
@AsyncStatus.wrap
async def set(self, value: int):
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉



@pytest.fixture
def smargon():
Copy link
Contributor

Choose a reason for hiding this comment

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

Could: There is a better sim motor now in ophyd_async.sim.demo.sim_motor which we could replace the Motors with instead

set_mock_value(device.disp, 1)
proc_mock = get_mock_put(device.proc)
proc_mock.return_value = NullStatus()
status: AsyncStatus = cast(AsyncStatus, device.set(1))
Copy link
Contributor

Choose a reason for hiding this comment

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

this cast is not necessary

@rtuck99 rtuck99 force-pushed the 601_convert_smargon_to_ophyd_async branch from 097b332 to 07ea06c Compare July 8, 2024 14:16
@rtuck99 rtuck99 merged commit 49c4ab1 into main Jul 8, 2024
11 checks passed
@rtuck99 rtuck99 deleted the 601_convert_smargon_to_ophyd_async branch July 8, 2024 14:26
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.

Convert smargon to ophyd_async
2 participants