-
Notifications
You must be signed in to change notification settings - Fork 28
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
Fix hamamatsu tiff reader if x axis not calibrated #347
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #347 +/- ##
=======================================
Coverage 87.84% 87.84%
=======================================
Files 85 85
Lines 11242 11246 +4
Branches 2082 2084 +2
=======================================
+ Hits 9875 9879 +4
Misses 868 868
Partials 499 499 ☔ View full report in Codecov by Sentry. |
pre-commit.ci autofix |
7b7d687
to
a3d2f8b
Compare
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 like a pretty simple bug fix!
@@ -696,7 +696,11 @@ def _get_hamamatsu_streak_description(tiff, op): | |||
scaling = dict_meta["Scaling"] | |||
|
|||
# Address in file where the X axis is saved | |||
x_scale_address = int(re.findall(r"\d+", scaling["ScalingXScalingFile"])[0]) | |||
# If x axis is "Other" it just loads the axis as pixels | |||
if scaling["ScalingXScalingFile"].startswith("Other"): |
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.
How does this fail currently. If for example the "ScalingXScalingFile" were set to "custom" would this fail again?
Maybe consider something like a try --> except in the else statement so that the data is still loaded but it throws a warning saying the scale could not be properly loaded.
I think in general we should try to always return everything we can get even if parts of the metadata aren't loaded properly becuase file formats are always a bit of a moving 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.
In principle yes, but normally it contains an address in the file where the axis is saved - if that is not the case it is set to "Other". That is not documented: http://www-bl20.spring8.or.jp/detectors/manual/HelpHiPic.pdf (Appendix D, E). However, I do not expect hamamatsu to have different keywords that can go there in place of an address, so I think it might even be better to get an error if there is yet another situation and not do a try and except for situations that are not documented.
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.
Generally speaking, I am on fence regarding @CSSFrancis's suggestion:
- in principle this is sensible, if there is a warning to suggest to report this issue to avoid a silent failure (here calibration not loading).
- in practise, raising an error motivates user to report error, while a warning will be ignored usually (and a source of noise or polluting log, notebook, etc. for some users) and it means that there is a risk that it doesn't get fixed!
add tests and changelog amend comment
a3d2f8b
to
ae06aa1
Compare
Description of the change
Fixes reading error when Hamamatsu tiff file contains
ScalingXScalingFile="Other"
and not an address in the file (not calibrated axis).Progress of the PR
upcoming_changes
folder (seeupcoming_changes/README.rst
),docs/readthedocs.org:rosettasciio
build of this PR (link in github checks)Minimal example of the bug fix or the new feature