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 ophyd component name kwargs when present #420

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ronpandolfi
Copy link

Ophyd components can have human-readable name kwargs. Using these rather than attribute names for labeling panel rows looks nice (see screenshot). This change defaults to the attr name when the name kwarg is absent.

image

The temp_celsius component has been given a name kwarg, and so its typhos panel displays this instead.

@ihumphrey @JulReinhardt

@klauer
Copy link
Contributor

klauer commented Feb 11, 2021

We've been back and forth on this one on our end, though I think our general consensus for the typhos screens has been "keep it consistent with the component (attribute) name you'd find on the IPython command-line."

Separately, though, Temperature (C) is a poor name for an ophyd component: it's not a valid Python identifier, it doesn't identify the object that it came from, and there may be limitations as to how that object can be included in a bluesky scan (I may be remembering incorrectly on the last one, I guess it depends on where you're shipping the scan data).

Perhaps we could consider a configurable option for this functionality if we can't come to an agreement here (@ZLLentz?)

@ZLLentz
Copy link
Member

ZLLentz commented Feb 11, 2021

I think there's definitely space in typhos for "nicer than the python attribute" names. To keep this consistent with normal best practices for ophyd/bluesky and the design ethos of typhos/happi:

  • We should show the python attribute name on mouseover for debugging purposes (the current one, that you use to access the signal object)
  • We should use something other than (or in addition to) the name field, because changing this field changes the way data is collected in bluesky, and the whole point of using systems like happi is so we can use the same device objects with the same args/kwargs in different applications. For example, setting the name of a signal to Temperature (C) strips off the parent device name from the data (presumably lakeshore_temp_celcius). This may be fine in this instance but I hesitate to say that it's fine in general, there should be an option for displaying a nicer name without interfering with bluesky.

@ronpandolfi
Copy link
Author

ronpandolfi commented Feb 16, 2021

I'm not seeing that adding the name kwarg affects bluesky. The field names and all other resulting documents are unaffected. The field name remains as lakeshore_temp_celsius. That value doesn't seem to make it into the document stream. I also don't see any limitation requiring that this kwarg be a valid Python identifier. The component is still referenced via its attribute name, rather than this kwarg.

Can you clarify the source of the limitations you were expecting?

@ZLLentz
Copy link
Member

ZLLentz commented Feb 16, 2021

Does setting name='my_favorite_string' not change the data output?

In [2]: from ophyd import Signal

In [3]: my_signal = Signal(name='temp_celcius', value=0)

In [4]: my_signal.read()
Out[4]: {'temp_celcius': {'value': 0, 'timestamp': 1613516704.955459}}

In [6]: my_signal.name = 'Temperature (C)'

In [7]: my_signal.read()
Out[7]: {'Temperature (C)': {'value': 0, 'timestamp': 1613516704.955459}}

@ZLLentz
Copy link
Member

ZLLentz commented Feb 16, 2021

In general, I'm thinking the behavior we want is something like:

  • Use the attribute name for signals defined like my_attribute_name = Cpt(Signal)
  • Use the first line of the doc field for signals defined like my_attribute_name = Cpt(Signal, doc='My Good Name\n Long text')
  • Discourage in-device changes of the name field because it affects the output of read

If you disagree, I think it's good to use the name field as long as it is a configurable. Ken is out today so I'll talk to him when he gets back in.

@ronpandolfi
Copy link
Author

ronpandolfi commented Feb 17, 2021

@ZLLentz I'd been attributing the name through the component. This is probably not intended usage after all.

temp_celsius = Cpt(EpicsSignalRO, 'TemperatureCelsius', name='Temperature (°C)')

I was expecting that the name kwarg here would get passed on to EpicsSignalRO, but its getting overwritten by the attribute name just before the signal is constructed.

Although this 'worked', its not an ordained attribute, and so this is probably the wrong approach. It seems like there's no proper place for a display name in ophyd signals. I would advocate for an addition to satisfy this need. The signal name must be a valid python identifier, so it isn't comfortable to read. My impression is doc is more of a long description.

@danielballan If there's no place for a human-readable name for signals, could one be (optionally) added?

@ZLLentz
Copy link
Member

ZLLentz commented Feb 17, 2021

Ah! That all makes sense now. I agree that there should be some ordained way in ophyd for a "nicer/readable" name, and maybe expanded metadata in general. @klauer has been building out some tooling for this that it might be time to propose in the ophyd repository.

@klauer klauer mentioned this pull request May 3, 2022
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