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

Add initial orchestrators idea #1169

Closed
wants to merge 1 commit into from
Closed

Add initial orchestrators idea #1169

wants to merge 1 commit into from

Conversation

marcomontevechi1
Copy link

Im not sure if the general ideia fits in ophyd scope, but this is an initial attempt.
The idea is to have abstractions to perform tasks that are possibly frequent to the plugins and drivers, but not basic enough to be put in the initial plugin/driver abstractions. For example, a common method to configure a grid of peaks in SimDetector or to one to define how to average images in processPlugin.

I wanted to set a default behavior in the method but in a way that parameter default values could be overriden by an user who knows what they are doing.

Unfortunately, the only way i could come up to do this was by manually coding the properties as possible keys to kwargs (eg. line 22 in ProcessPluginOrchestrator). I tried dynamically checking if any argument in kwargs was equal to one of the component_names but for that i needed to use something like getattr(self.cam, component_name).put() and for some reason that didnt work.

Im not sure if the general ideia fits in ophyd scope, but this is an
initial attempt.
The idea is to have abstractions to perform tasks that are possibly
frequent to the plugins and drivers, but not basic enough to be put
in the initial plugin/driver abstractions. For example, a common
method to configure a grid of peaks in SimDetector or to one to
define how to average images in processPlugin.

I wanted to set a default behavior in the method but in a way that
parameter default values could be overriden by an user who knows what
they are doing.

Unfortunately, the only way i could come up to do this was by manually
coding the properties as possible keys to kwargs (eg. line 22 in
ProcessPluginOrchestrator). I tried dynamically checking if any argument in
kwargs was equal to one of the component_names but for that i needed to
use something like getattr(self.cam, component_name).put() and for some
reason that didnt work.

# Overriding with kwargs part
properties = {
kwargs.get("enable_background"): self.enable_background,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is going to make the keys the values in kwargs (many of which will be None). I think what you want is something like

for k, v in kwargs:
    getattr(self, k).put(v)

Copy link
Author

Choose a reason for hiding this comment

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

I tried something similar to:

for k in kwargs.values():
    getattr(self.cam, k).put(kwargs[k])

but for some reason the operation completed without errors but also without changing the PV values.

I tried testing again now but for some reason, cloning my own fork, installing it with pip and testing it with the same SimDetector IOC container image i used before throws an NotImplementedError every time i try to execute a put operation... what am i doing wrong?

>>> from ophyd.areadetector.detectors  import SimDetector
>>> s = SimDetector(prefix="SIM:", name="SIM")
>>> s.cam.acquire.put(1)
Traceback (most recent call last):
  File "/home/marco/.local/lib/python3.10/site-packages/ophyd/device.py", line 325, in __get__
    return instance._signals[self.attr]
KeyError: 'acquire'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/marco/.local/lib/python3.10/site-packages/ophyd/device.py", line 327, in __get__
    return instance._instantiate_component(self.attr)
  File "/home/marco/.local/lib/python3.10/site-packages/ophyd/device.py", line 1352, in _instantiate_component
    self._signals[attr] = cpt.create_component(self)
  File "/home/marco/.local/lib/python3.10/site-packages/ophyd/device.py", line 266, in create_component
    cpt_inst = self.cls(pv_name, parent=instance, **kwargs)
  File "/home/marco/.local/lib/python3.10/site-packages/ophyd/areadetector/base.py", line 36, in __init__
    super().__init__(prefix + "_RBV", write_pv=prefix, **kwargs)
  File "/home/marco/.local/lib/python3.10/site-packages/ophyd/signal.py", line 1740, in __init__
    super().__init__(
  File "/home/marco/.local/lib/python3.10/site-packages/ophyd/signal.py", line 971, in __init__
    self._read_pv = self.cl.get_pv(
  File "/home/marco/.local/lib/python3.10/site-packages/ophyd/_dummy_shim.py", line 51, in get_pv
    raise NotImplementedError
NotImplementedError

shouldnt this be implemented in the base class?

@tacaswell
Copy link
Contributor

Could these be better handled as plans?

@marcomontevechi1
Copy link
Author

@tacaswell yes, now that you mention i think its a purpose more suited for a bluesky plan. Probably the class should be created there, also?
Waiting for an answer about the NotImplementedError before i close the PR.

@tacaswell
Copy link
Contributor

I don't think you need to create any classes.

for k, v in kwargs.items():
    yield from bps.set(getattr(obj), k), v, group='my_group')
yield from bps.wait(group='my_group')

(or yield from bps.mv(obj1, v1, obj2, v2, ...) but I can't write the flattening on-liner off the top of my head)

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