-
Notifications
You must be signed in to change notification settings - Fork 6
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
Add support for TOF imaging #309
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## next #309 +/- ##
==========================================
- Coverage 94.62% 93.73% -0.90%
==========================================
Files 21 21
Lines 1266 1277 +11
==========================================
- Hits 1198 1197 -1
- Misses 68 80 +12 ☔ View full report in Codecov by Sentry. |
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.
Small comments and changes. One bigger question is if you use mypy or just have the type annotations?
if all(matches): | ||
logger.info("Using rotation angles from filenames.") | ||
rotation_angles = np.array([float(".".join(m.groups())) for m in matches]) | ||
regex = r"\d{8}_\S*_\d{4}_(?P<deg>\d{3})_(?P<dec>\d{3})_\d*\.(?:tiff?|fits)" |
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 add tests for this function to press on this regex a lot harder
@@ -103,6 +103,7 @@ def calculate_dissimilarity( | |||
tilt: float, | |||
image0: np.ndarray, | |||
image1: np.ndarray, | |||
center: Optional[Tuple[Union[float, int], Union[float, int]]] = None, |
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.
This would be more readable if you created a custom type. Maybe
type Number = Union[float, int]
type Pair = Tuple[Number, Number]
...
center: Optional[Pair] = None,
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.
it will be easier once we make 3.11 the minimum version of python as we can write it as Optional[Tuple[float|int, 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.
I think that the compound typing you have is harder to read. You can keep it how you want, but center: Optional[Tuple[Number, Number]] = None
is significantly clearer to me.
@@ -124,6 +124,24 @@ def test_normalization_bright_dark(): | |||
assert diff < 0.01 | |||
|
|||
|
|||
def test_normalization_no_darks(): |
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.
Do you know about numpy.testing?
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.
I am assuming you are asking to use np.testing.assert_all_close instead of computing the average difference here. They are similar in function, but I prefer the explicit one here over the one provided by numpy
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.
Just a thing to point out as existing
provide file type in error messages as suggested Co-authored-by: Pete Peterson <[email protected]>
use consistent type hinting Co-authored-by: Pete Peterson <[email protected]>
Just the type annotations. We are not using mypy for iMars3D. |
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.
I think adding mypy to this repository will make you have to work a bit more on your type annotations. The code looks reasonable.
This PR introduces this following changes:
memmap
(the instrument team reported that it is slowing down the reading).tiff
and.tif
are considered valid extension for TIFF input format nowdarks=None
to assume perfect dark fields (all dark fields are zero)None
if extraction fails. For files with partially missing rotation angles, the loader will returnnp.nan
in the rotation angle list, allowing users to handle or sanitize the missing values as needed. While files must still adhere to the correct naming pattern (or contain the necessary metadata) for auto reduction, the auto reconstruction process will no longer fail if rotation angle extraction is unsuccessful.