-
Notifications
You must be signed in to change notification settings - Fork 30
Generalize beam shift deflector and stage rotating speed interface #124
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?
Conversation
… match already-used wording
…instead of `np.int`
@viljarjf Tagging you because you are the author of the |
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 good to me, I left a minor comment.
@@ -145,6 +145,10 @@ def xy(self, values: Tuple[int_nm, int_nm]) -> None: | |||
x, y = values | |||
self.set(x=x, y=y, wait=self._wait) | |||
|
|||
def get_rotation_speed(self) -> Union[float, int]: |
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.
def get_rotation_speed(self) -> Union[float, int]: | |
def get_rotation_speed(self) -> Number: |
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.
Can do though it will require adding declaration there as well. In the other place it was more needed since this patter repeated multiple times. 👍
Alright, done. If that is the case and there are no further comments, I will merge this tomorrow. |
Context
Below I would like to suggest some small changes to the interface that may warrant an adequately short discussion:
Firstly, the
Deflector
class, parent forBeamShift
,BeamTilt
,DiffShift
etc. is type-hinted to accept and return only instances ofint
. However, FEI microscopes do not store these as integers, but rather floats between -1 and 1 exclusive. This carries major implications because some functions, e.g.CalibBeamShift.pixelcoord_to_beamshift
explicitly map its result toint
usingbeamshift.astype(int)
. rendering use on FEI machine impossible. So here I suggest modifyingmicroscope/component/deflectors:Deflector
base class hints to suggest that these values can be of either numeric type.Secondly, although following the above
CalibBeamShift
should theoretically return shifts asint
, it does not. The return type ofpixelcoord_to_beamshift
and the setting methods use as a containernp.ndarray
full ofnp.int
. Consequently, if your server does not have numpy installed,np.int
is an unknown type. This can be easily fixed by mapping all set values to float:ctrl.beamshift.set(*(float(b) for b in beamshift))
. This completely fixes the issue for FEI, and should be benign for Jeol since its interface converts it back toint
anyway, and the precision of converting int to float and back is likely good enough:Thirdly, the
Stage
class currently has two (unused) methods to work with stage speed: theset_rotation_speed
setter and therotating_speed
context. I suggest adding methodget_rotation_speed
- otherwise, one can get it only via a camel case parameter of a private attribute. I also suggest renamingrotating_speed
torotation_speed
so that it matches the other two. I could not find this context used anywhere outside my own code, so I doubt anyone will mind.Finally, I have introduced here some QOL improvements: made imports absolute (otherwise calibration could not be run as script, plus relative imports are good only in dynamically developed packages, link), added or improved type hinting, made it so that you don't need to manually make a folder to save a tiff file during calibration.
Minor change
CalibBeamShift
: allow saving tiff in a new directory so that calibration can be run without manually creating the output directory for collected tiff files each time.Bugfix
CalibBeamShift
: some methods return or setfloat
instead ofnp.int
type, so that you can use this calibration of FEI;calibrate_beamshift_live
: can now be run as a script thanks to making imports absolute;calibrate_beamshift_live
: fix typo so that calibration files are not all saved under the same namecalib_beamshift_{i.tiff
.Code maintanance
Deflector
child classes are now properly type hinted to handle eitherint
orfloat
type values.