Add long names#627
Conversation
ZLLentz
left a comment
There was a problem hiding this comment.
I like the way this is implemented. I think the docs inclusion is required before merge and the other suggestion is optional.
| # Hacky workaround until Ophyd.Component.long_name PR comes through | ||
| long_name = None | ||
| try: | ||
| if hasattr(getattr(device, attr), 'long_name'): | ||
| long_name = getattr(device, attr).long_name | ||
| except AttributeError: | ||
| # Then maybe we have a nested component and can't touch the signal | ||
| if hasattr(getattr(device, dotted_name), 'long_name'): | ||
| long_name = getattr(device, dotted_name).long_name |
There was a problem hiding this comment.
If this bit is a hacky workaround, and we needed it twice, it should be pulled out into its own function. After pulling out into its own function this is a reasonable thing to write a short unit test for to make sure we've covered all the cases we expect.
I'm wondering if there's some way to revise this to make it more readable but I don't have any immediate suggestions. Some of the hasattr calls probably aren't needed I guess.
There was a problem hiding this comment.
Agreed, it's probably worth some:
def _get_long_name(self, device, attr, dotted_name) -> str:
"""
Check if the signal has implemented a long_name and return it. Will check both the device and the component signal for the long_name if it exists.
"""
long_name = None
try:
if hasattr(getattr(device, attr), 'long_name'):
long_name = getattr(device, attr).long_name
except AttributeError:
# Then maybe we have a nested component and can't touch the signal
if hasattr(getattr(device, dotted_name), 'long_name'):
long_name = getattr(device, dotted_name).long_name
return long_name
I think I ended up needing both hasattr due to it getting squirrelly when you build a screen with multiple component devices where the long name is actually in device.component.attr.long_name instead of device.attr.long_name — and the fact that long_name is not in the Ophyd object by default yet. Once they implement it we can probably just replace it with some try block around if getattr(device, attr).long_name instead.
There was a problem hiding this comment.
Missing here is the pre-release notes docs step, same as how we've done it in pcdsdevices with pre-release-notes.sh
There was a problem hiding this comment.
Ah derp forgot to commit that, let me fix that after above changes
| try: | ||
| if hasattr(getattr(device, attr), 'long_name'): | ||
| long_name = getattr(device, attr).long_name | ||
| except AttributeError as ex: |
There was a problem hiding this comment.
Small note: the pre-commit CI job is flagging ex as an unused variable and a couple instances of trailing whitespace
| device: (any) | ||
| The Ophyd.Device with component signals | ||
| attr: (str) | ||
| The str name of the signal attribute | ||
| dotted_name: (str) | ||
| The full dotted name to the signal | ||
|
|
||
| Returns | ||
| -------- | ||
| str or None |
There was a problem hiding this comment.
Super petty spacing nitpick to match the other docstrings
| device: (any) | |
| The Ophyd.Device with component signals | |
| attr: (str) | |
| The str name of the signal attribute | |
| dotted_name: (str) | |
| The full dotted name to the signal | |
| Returns | |
| -------- | |
| str or None | |
| device: (any) | |
| The Ophyd.Device with component signals | |
| attr: (str) | |
| The str name of the signal attribute | |
| dotted_name: (str) | |
| The full dotted name to the signal | |
| Returns | |
| -------- | |
| str or None |
There was a problem hiding this comment.
I don't think this ever got committed 👀
There was a problem hiding this comment.
I don't even remember getting this message on the commit o.O
| label.setToolTip(_tooltip) | ||
| return label | ||
|
|
||
| def _get_long_name(self, device, attr, dotted_name) -> str: |
There was a problem hiding this comment.
More pettiness: I normally wouldn't ask you to annotate, but this one is half annotated. Can you either finish the annotations or remove them?
device: typing.Any
attr: str
dotted_name: str
-> str | None
There was a problem hiding this comment.
I'll remove the annotations to be more consistent with the rest of the script, although it probably wouldn't hurt to add the annotations to all of the funcs one day
Co-authored-by: Zachary Lentz <ZLLentz@users.noreply.github.com>
ZLLentz
left a comment
There was a problem hiding this comment.
I've only got nitpicks left
If you're happy and finished we're good to go
I still like the implementations here and the result
Should pass the checks now, gave it the ole flake8 rundown. Passing |
ZLLentz
left a comment
There was a problem hiding this comment.
Looks good to me, let's roll
Remaining CI is an unrelated weird python 3.11 race condition that I don't need to think about today
Description
Made changes to
SignalPanelclass indisplaymodule to supportlong_namesfor device signals. This approach should be pretty close to compatible with bluesky/ophyd#1187, so if & when that comes through we should only need minimal changes.Motivation and Context
Have you ever had a screen with 30+ signals and maybe multiple subcomponent devices with identical signal names? Maybe you wanted alternative signal labels for cleaner GUI that end users can play with?
Figured some of the work I did in pcdshub/pcdsdevices#1320 could be generalized in
typhosand potentially trim the fat out of my screen scripts.Long names lets you use an optional string to replace the
label_textin asignal rowobject added to your grid of signals. When you use along_name, it will add the signal'sdotted_nameto the tooltip so you can still hover and get hutch python context.How Has This Been Tested?
I didn't have any unit tests to run, per se, but I did test this on a variety of devices for screens we've got out in prod (and some I'm cooking in a different PR).
I was able to open the entire CRIXS MODS screen without everything crashing and burning, so that's gotta amount to something, right?
test_typhos_long_name_crixs_mods.mp4
Where Has This Been Documented?
This PR! And in the code itself
Pre-merge checklist
docs/pre-release-notes.shand created a pre-release documentation page