Skip to content

Allow constants in kwargs of derived_signal #859

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

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

shihab-dls
Copy link
Contributor

Fixes #857

This PR should allow literals of type int | bool | str | float to be passed in as raw_devices. The approach that I've taken is filtering out these constants in SignalTransformer, then passing them back in when a derived reading is made. There are probably more streamlined ways of doing this, so this is an initial pass to check that this is the intended functionality.

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.

The typing is all a bit messy but I'm not sure if there's a better way to do it. Functionally works great, just the one comment. Also, could you add something to https://blueskyproject.io/ophyd-async/main/how-to/derive-one-signal-from-others.html?

Copy link
Collaborator

@coretl coretl left a comment

Choose a reason for hiding this comment

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

Neat! A couple of suggestions to tidy...

@shihab-dls
Copy link
Contributor Author

If we have a multi derived signals using transforms, we still need to pass a signal as a parameter, as this will be a transform_device. Splitting into raw_constants, raw_devices, transform_constants, and transform_devices lets us unpack raw_constants when reading raw_values, and unpack transform_constants when populating the transform with args. This should thus be general for passing constants in single and multi derived signals. So the order of splitting this up is now:

  • Take raw_and_transform_devices_and_constants in DerivedSignalFactory
  • Split into _raw_and_transform_devices and _raw_and_transform_constants for checking against transform_cls, as this lets us type(v) for constants and v.datatype or get_locatable_type(v) for devices.
  • Split into raw_constants, raw_devices, transform_constants, and transform_devices before passing to SignalTransformer

Then when we collect our derived readings we can:

raw_values = {
    **{
        k: raw_and_transform_readings[sig.name]["value"]
        for k, sig in self._raw_devices.items()
    },
    **self._raw_constants,
}

and

return self._transform_cls(**(transform_args | self._transform_constants))

in _make_transform_from_readings. So, given a single derived signal, a constant would be populated as a raw_constant, and no transform_constant is passed as no specific transform is given. And, given a multi derived signal, a constant would be populated as a transform_constant and no raw_constant is expected (i.e., parameters should be passed as type hints to the transform, and thus a transform_constant).

@shihab-dls shihab-dls requested a review from coretl May 9, 2025 13:09
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.

Allow constants in kwargs of derived_signal
3 participants