-
Notifications
You must be signed in to change notification settings - Fork 31
Improve typing, docstrings, convenience, DRY in stagematrix calibration #149
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
Improve typing, docstrings, convenience, DRY in stagematrix calibration #149
Conversation
stefsmeets
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 work, looks good! Left a minor comment.
How do you test the typing? We don't have mypy or similar set up.
|
|
||
| np.set_printoptions(suppress=True) | ||
|
|
||
| Mode = Literal['mag1', 'mag2', 'lowmag', 'samag'] |
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.
Enums are also nice for this. I find literal types to be a pain to work with, because you must always pass a literal for this to pass. For example, this will fail mypy:
mode = 'mag1'
get_or_roughly_estimate_pixelsize(mode=mode, ...)
# error: Argument 1 to "get_or_roughly_estimate_pixelsize" has incompatible type "str"; expected "Literal['mag1', 'mag2', 'lowmag', 'samag']" [arg-type]So you must always define mode = Literal['mag1'].
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.
Wow, that sounds absurd. I understand the logic but it does limit the usefulness of Literal indeed. But also I learned that apparently you can use Final (introduced in python 3.8, ~equivalent to Java final) to suppress it:
from typing import Final
mode: Final[str] = 'mag1'
get_or_roughly_estimate_pixelsize(mode=mode, ...) # passes, apparentlyI find it funny how the typing module, especially with mypy, morphs Python into a static-typed language, like a software carcinization 🤣.
Regarding type checking, I am personally relying on the checker built-in PyCharm, it updates live and is not perfect but quite good. But I use typing mostly as a matter of preference rather than a layer of security, IMO it makes understanding code so much easier. My recent go-to is short docstrings and well-named and type-hinted attributes.
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.
Added some changes suggested by mypy. I actually did not get any problems with Literal / str, though wow, "fixing" entirety of instamatic to be accepted by mypy, especially --strict would be "fun".
Merging by ~Wednesday unless anyone voices issues.
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.
Yeah, basedpyright or the now deprecated basedmypy are nice for this, because they record a 'baseline' of issues to work with, so you only get warnings on new code.
I needed to use the "stagematrix" calibration, i.e. a matrix to (roughly) translate between camera and stage coordinates. I found the code mostly working, but I thought it could use more docs and less repetition. I would like to suggest the following changes. I did not introduce changes other than a few minor convenience features described below.
All the changes below concern file
src/instamatic/calibrate/calibrate_stagematrix.pyunless stated otherwise.Minor changes
pixelsizeis undefined for a givenmag, try toget_or_roughly_estimate_pixelsizeit based on other pixel sizes in givenmode, assuming the product ofmagandpixelsizeshould be constant (useful forcalibrate_stage_all);src/instamatic/utils/iterating.py: Addpairwisethat iterates over pairs of subsequent elements e.g.pairwise([1, 2, 3, 4])yields(1, 2),(2, 3), and(3, 4)(built in Python 3.10 iterable, can be removed alongside Python 3.9 support).Bug fixes
instamatic.caliubrate_stagematrixoption--all_mags.Code maintenance
Literalmode hints for TEM modes;calibrate_stage_from_*'s "args") to be more meaningful;plot_resultswrite_tifffunctionality built inget_imageover a separate import/op;