-
Notifications
You must be signed in to change notification settings - Fork 2
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
Choose the detector to use and move the detector stage #58
Conversation
Codecov Report
@@ Coverage Diff @@
## main #58 +/- ##
==========================================
+ Coverage 47.56% 47.92% +0.36%
==========================================
Files 19 21 +2
Lines 3267 3309 +42
==========================================
+ Hits 1554 1586 +32
- Misses 1713 1723 +10
📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today! |
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.
Looks fine, just a couple comments
def setup_detector_stage(detector_stage: DetectorMotion, expt_type: str): | ||
# Grab the correct PV depending on experiment | ||
# Its value is set with MUX on edm screen | ||
det_type = pv.me14e_gp101 if expt_type == "fixed-target" else pv.ioc12_gp15 |
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.
Should: can we use the SSXType enum for this?
|
||
def get_detector_type() -> Detector: | ||
det_y = caget(pv.det_y) | ||
# Note to self, I should also be able to use detmotion for this too! |
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.
Should: please expand into an issue and link it so that someone else could fix this, or remove
parser.add_argument( | ||
"expt", | ||
type=str, | ||
choices=["extruder", "fixed-target"], |
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 should be able to parse directly to SSXType enum here if the string choices match the enum values
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.
Great, thank you! I realise my comment wasn't very clear, so I made a small change to show what I meant. Please review it and make sure you're ok with it before merging
Closes #50
Adds a small plan to move the detector stage to the correct position depending on the requested detector to use.
Also add the edm screens for fixed-target and extruder to trigger it.