add XCS ladm.py file and add tab whitelist/alias to lens.py #1316
Conversation
There was a problem hiding this comment.
Thank you for bringing this into the main pcdsdevices!
I made a lot of comments here. I marked each one to help note the relative importance of each.
Change requests are things that definitely need to be done before I can approve the PR.
Questions and confusions really need a response so we can figure out together if something needs to be done or not.
Suggestions are things that I highly recommend doing but I won't hold back the PR for.
You can ignore or defer follow-up ideas and nitpicks if you don't have time, even though I think they are at least somewhat useful.
| alpha_cos = np.cos(27 * np.pi/180) | ||
|
|
||
|
|
||
| def ThetaToMotors(theta, samz_offset=0): |
There was a problem hiding this comment.
Nitpick:
This file commits a lot of pep8 sins, I'm not going to have the bandwidth to point them out one by one but not following pep8 makes the code more annoying for python devs.
https://peps.python.org/pep-0008/
| """ | ||
|
|
||
| # Motors | ||
| x1 = Cpt(IMS, ':MMS:01', name="x1", kind='normal', |
There was a problem hiding this comment.
Nitpick:
It's pretty strange that this is generalized to such a degree. I think there's only one LADM and it has a fixed prefix. It doesn't need to be fixed but it's possible to e.g. set a default class prefix.
| self.theta = None # VirtualMotor | ||
| self.XT = None # VirtualMotor | ||
| self.gamma = None | ||
| self.__lowlimX = None | ||
| self.__hilimX = None |
There was a problem hiding this comment.
Confusion/suggestion/question:
Are any of these variables actually used in a meaningful way?
Elsewhere in the class some of these are essentially unused, others are referenced as if they aren't None but they also aren't set anywhere, so there's no way to make them values other than None without some runtime manipulation by the user.
If they aren't used, we should remove them.
If they are used, we should set them properly, either in here if we have all the info or as an __init__ argument to ensure they get included at init time.
| print(self.theta.position()) | ||
| theta_now = self.theta.position() |
There was a problem hiding this comment.
Confusion:
Here, self.theta, which is nominally None, is assumed to have a position() callable, but on line 190 it is assumed to have a position attribute containing the raw position value. It can't have both.
| except KeyboardInterrupt: | ||
| self.stop() | ||
|
|
||
| def wmX(self): |
There was a problem hiding this comment.
Suggestion:
It's worth taking a moment to check each of the methods here to make sure they have at least something useful in the docstring.
pcdsdevices/lens.py
Outdated
There was a problem hiding this comment.
Change request:
This PR doesn't have any pre-release notes, and it needs them.
There was a problem hiding this comment.
Stretch goal/follow-up idea:
It'd be nice to have some basic unit tests that create a fake device and run through the most used functions.
|
|
||
| alpha_r = 63 * np.pi/180 # rail angle | ||
| r = 2960.0 # first rail distance from sample to rail rod at 27deg | ||
| R = 6735.0 |
There was a problem hiding this comment.
What is this? Surely there's a more descriptive name than captial R (to avoid confusion with lowercase r)
There was a problem hiding this comment.
I'm guessing it is another rail distance, but I'm not sure who wrote this code...
| return x1, x2, dz | ||
|
|
||
|
|
||
| def y1ToGamma(y1): |
There was a problem hiding this comment.
Why are these defined outside the LADM class? They seem to be rather specific to the geometry of the LADM, with coordinate names that only someone familiar with the LADM would understand.
Generally I dislike such opaque naming. Without a diagram it's impossible to verify that this math is doing the right thing
| except KeyboardInterrupt: | ||
| self.stop() | ||
|
|
||
| def mvrV(self, y): |
There was a problem hiding this comment.
Enhancement / Improvement idea / Musing:
It looks like we're most interested in derived quantities (V, H, X, Theta), rather than the motors that have been defined in this class (x1, y1, z, etc)
Should we be wrapping these quantities in pseudo positioners so that we can use normal manipulate them with normal FltMvInterface methods?
| self.y2.stop() | ||
| self.z.stop() | ||
|
|
||
| def __call__(self): |
There was a problem hiding this comment.
Nitpick: We probably want to use the standard format_status_info method that displays device information in ipython. That way you don't actually have to write the parentheses () when you want to see the status info.
More concisely, I think if we define LADM.format_status_info we should just be able to type LADM in the ipython terminal to see the status info
Co-authored-by: Zachary Lentz <[email protected]>
Co-authored-by: Zachary Lentz <[email protected]>
Co-authored-by: Zachary Lentz <[email protected]>
Co-authored-by: Zachary Lentz <[email protected]>
Description
Adding ladm.py file and tab whitelists/aliases names in lens.py
Motivation and Context
XCS ladm.py file has been copied manually into pcdsdevices upgrades, but it would be helpful to have it as part of the git directory.
Also tab whitelists/aliases used in XCS in the lens.py file are helpful.
How Has This Been Tested?
Has been running in XCS pcdsdevices.
Where Has This Been Documented?
Here
Pre-merge checklist