pcdshub / lightpath

LCLS Lightpath Module
https://pcdshub.github.io/lightpath
Other
4 stars 9 forks source link

ENH: support devices with multiple simultaneous outputs #160

Closed tangkong closed 1 year ago

tangkong commented 1 year ago

Description

Motivation and Context

LODCM's support multiple simultaneous outputs, and the current lightpath did not.

BeamPath.path is called by almost every method / property (.blocking_devices, .incident_devices, etc), and previously sorted the devices every time.

The helper method (get_device_output) added here would have needed to find a given device's index in that path list, which scales poorly as the number of devices in a single path increases. This gets worse as we iterate through path. I figure if we do this calculation once, we can save time down the line.

Also, why the very specific OrderedDict, mapping a device name to its successor device? I could have put things into a simple linear DiGraph or made a proper linked list, but all that seemed excessive for the problem I was trying to solve. I'm open to other idea here in particular (as well as across the rest of this change)

How Has This Been Tested?

Test suite runs, additional test has been added.

Interactive mucking around works too

The final test is to go through my pcdsdevices branch and change all that logic, which has been confirmed to not horribly destroy lightpath! 🎉

Where Has This Been Documented?

docs have been updated

image image
tangkong commented 1 year ago

Is there any specific choice you want to talk more about?

The only bit I was unsure about was the change to LightpathState.output, but I think we've settled on that as a reasonable route forward. Thanks for the review 👍