ElectronAnalyser no longer depends directly on sequence file path to work with BlueAPI#1923
ElectronAnalyser no longer depends directly on sequence file path to work with BlueAPI#1923oliwenmandiamond wants to merge 7 commits intomainfrom
Conversation
…work with BlueAPI
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1923 +/- ##
==========================================
- Coverage 99.05% 99.05% -0.01%
==========================================
Files 312 312
Lines 11754 11735 -19
==========================================
- Hits 11643 11624 -19
Misses 111 111 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
f807817 to
7d4d359
Compare
| ): | ||
| 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.
this is a driver that is created and passed in a child class, here in parent class it is taken out from the controller and saved as a class object - wouldn't it be clearer to make it in a child class directly when driver is created?
There was a problem hiding this comment.
The class is then in charge of making sure the driver and detector connect and parent/name relationship is correct rather than the API. It's much better to make sure the API does it as all classes will need to do this, otherwise we copy and paste this line x number times for number of implementations which is unnecessary.
There was a problem hiding this comment.
you probably won't need to copy paste anything just add "self.driver" instead of making internal "driver" in a child class
There was a problem hiding this comment.
The issue with this is that if we type anything as using the GenericElectronAnalyser for tests or plans or other devices i.e it doesn't care about if it Specs, Mbs, VGScienta etc. then it loses access to the driver because it lives now on the implementation and not on the base. We need it to be done here.
| @@ -201,7 +187,7 @@ def create_region_detector_list( | |||
|
|
|||
|
|
|||
| GenericElectronAnalyserDetector = ElectronAnalyserDetector[ | |||
There was a problem hiding this comment.
do you need this class here? it seems to be used only in tests
There was a problem hiding this comment.
Yeah, this will be used in plan repo too.
There was a problem hiding this comment.
which plan repo? sm-bluesky?
There was a problem hiding this comment.
Fixes #1924
Instructions to reviewer on how to test:
Checks for reviewer
dodal connect ${BEAMLINE}