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

Changes to support hyperion 1283 use pixels per micron from file not gda #423

Merged
merged 4 commits into from
Apr 23, 2024

Conversation

rtuck99
Copy link
Contributor

@rtuck99 rtuck99 commented Apr 8, 2024

Fixes (DiamondLightSource/hyperion#1283)

Add microns-per-pixel to the snapshot so that oav snapshot events can contain data needed for ispyb
A small change to OAV initialiser to allow parameters to be specified on construction for system tests

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

@rtuck99 rtuck99 requested a review from d-perl April 8, 2024 09:35
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.

Just one nitpick, up to you

def oav(
wait_for_connection: bool = True,
fake_with_ophyd_sim: bool = False,
params=OAVConfigParams(ZOOM_PARAMS_FILE, DISPLAY_CONFIG),
Copy link
Contributor

@d-perl d-perl Apr 8, 2024

Choose a reason for hiding this comment

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

Nit: the default arg will be evaluated at definition time (i.e. on import). I know that just stores the paths, but I'd still rather do something like:

@skip_device(lambda: BL == "s03")
def oav(
    wait_for_connection: bool = True,
    fake_with_ophyd_sim: bool = False,
    params: OAVConfigParams | None = None,
) -> OAV:
    """Get the i03 OAV device, instantiate it if it hasn't already been.
    If this is called when already instantiated in i03, it will return the existing object.
    """
    return device_instantiation(
        OAV,
        "oav",
        "",
        wait_for_connection,
        fake_with_ophyd_sim,
        params=params or OAVConfigParams(ZOOM_PARAMS_FILE, DISPLAY_CONFIG),
    )

In case the OAVConfigParams ever changes to do something in its init

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'll make the change, although I think most classes should not do anything interesting in constructors, especially parameter classes.

@rtuck99 rtuck99 force-pushed the hyperion_1283_use_pixels_per_micron_from_file_not_GDA branch from 00388e4 to 0aeff28 Compare April 12, 2024 12:12
@rtuck99 rtuck99 merged commit fa03f7d into main Apr 23, 2024
11 checks passed
@rtuck99 rtuck99 deleted the hyperion_1283_use_pixels_per_micron_from_file_not_GDA branch April 23, 2024 14:51
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.

2 participants