Make sure no cast warning is issued when saving #149
Conversation
Includes a refactor of one big test method
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #149 +/- ##
==========================================
+ Coverage 91.23% 91.60% +0.37%
==========================================
Files 11 12 +1
Lines 3867 4004 +137
==========================================
+ Hits 3528 3668 +140
+ Misses 339 336 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
If it comes down to it, I'm OK merging this as is. However, I had one inline comment and if you have any time I'd like a review of methods like:
And see if something in those should be updated to do this fillna there.
Otherwise, could we get something in the docstring (maybe the main save at least?) about the behavior of NaNs/NA handling? Normally I wouldn't mind for such a small change, but as mentioned on slack this is really defining a previously undefined behavior so it'd be nice to document it.
| @@ -683,6 +683,8 @@ def _scale_to_dtype(self, data, dtype, fill_value=None): | |||
| data = data.clip(0, 1) * scale + offset | |||
| attrs.setdefault('enhancement_history', list()).append({'scale': scale, 'offset': offset}) | |||
| data = data.round() | |||
| if fill_value is None: | |||
| data = data.fillna(np.iinfo(dtype).min) | |||
There was a problem hiding this comment.
Correct me if I'm wrong, but while looking for where this _scale_to_dtype method is used I noticed there is a spot where right after this method is called the _replace_fill_value method is called and I think when the second argument (ifill) is passed as None it performs the equivalent of what you've done here. We may want to reuse that method. However, you'd still have to do the iinfo so maybe this isn't worth it.
|
This PR just got more interesting to me. I have a local branch where I'm trying to remove all test warnings. I ran into the ones fixed here and remembered this PR existed. What were you thoughts on my comment(s)? |
They make sense, but I didn't have the time to investigate further yet. |
|
I spent a lot of time looking at this and how the scale method is called in different ways and I couldn't find a way to refactor this stuff without starting from scratch. At first I was able to reuse some of the logic, but ran into issues when this I did make a change to how the test was working. When working out what was failing I got annoyed that the existing warning check was happening at the end of the test function rather than the actual |
This PR ensures no cast warning is raised when saving an image that contains nans to int.
Includes a refactor of one big test method.
git diff origin/master **/*py | flake8 --diff(remove if you did not edit any Python files)