-
Notifications
You must be signed in to change notification settings - Fork 12
Implement concrete analyser classes for beamlines to remove generics #1821
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
base: main
Are you sure you want to change the base?
Implement concrete analyser classes for beamlines to remove generics #1821
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1821 +/- ##
=======================================
Coverage 99.09% 99.09%
=======================================
Files 303 306 +3
Lines 11446 11486 +40
=======================================
+ Hits 11342 11382 +40
Misses 104 104 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
to beamline implementation instead.
…sh://github.com/DiamondLightSource/dodal into Implement_concrete_analyser_classes_for_beamlines
Villtord
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Surprisingly it is less code that I expected for moving away from more generics classes.
| ): | ||
| self._sequence_class = sequence_class | ||
| # Save on device so connect works and names it as child | ||
| self.driver = controller.driver |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you want it maybe to be more of internal object self._driver?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think having access to the driver is okay. Beamline scientist can then still read and move individual signals in their scripts e.g i09 example, by doing ew4000.driver.lens_mode instead of using having to expose this as another object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but without this line it would be ew4000.controller.driver.lens_mode - not a big difference, ideally it should be probably just ew4000.lens_mode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
controller is saved as self._controller so is private.
We can shorten it and have it saved drv so is ew4000.drv.lens_mode
| shutter: FastShutter | None = None, | ||
| name: str = "", | ||
| ): | ||
| driver = B07SpecsAnalyserDriverIO(prefix) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happens when you later call super().init(...) i? will it then overwrite this driver object with self.driver = controller.driver from ElectronAnalyserDetector class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The scope of driver in this class is only inside __init__ as it isn't saved to self and is passed directly to the parent class. Inside the parent class it will save it as self.driver so is accessible via analyser.driver and participates with naming and connect. So there is no overriding or classing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could then just create it without naming directly in B07ElectronAnalyserController
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean like this?
controller = B07ElectronAnalyserController(
B07SpecsAnalyserDriverIO(prefix),
energy_source,
shutter
)
super().__init__(controller, name)Yes I could do it like this, but think it is much cleaner to do the above (and less lines?)
Remove the generics from the analysers by implementing analyser models for each analyser type. This will also allow for beamline specific behaviour to be later implemented on an analyser.
Instructions to reviewer on how to test:
Checks for reviewer
dodal connect ${BEAMLINE}