-
Notifications
You must be signed in to change notification settings - Fork 301
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
Keep original dtype in DayNightCompositor #2640
Conversation
The tests will require pytroll/trollimage#150 |
The composite tests will require also pytroll/trollimage#151 and a new release of Trollimage. |
The changes in Trollimage might have affected the "thruth" values in also other tests, I'll have a look. |
2ff0b2a should fix three of the tests that were affected by floating point inaccuracy. There's a remaining Ninjo geotiff failure and three CF failures, I'd guess they are again because of a new XArray release without the compression fix. |
Oh, now there's only the single Ninjo tiff failure. I'll compare the results with previous Trollimage version with the current one tomorrow and see what could be the problem. I don't know what the |
Some background info to the failing test: The I have no clue what that has to do with dtype preservation in the |
Nothing, the change is due to my updates to Trollimage (most likely pytroll/trollimage#150) which were released today and needed for the DNC tests to pass in this PR. |
Ok. #!/usr/bin/env python
import numpy as np
import xarray as xr
from trollimage import xrimage
arr = np.arange(75, dtype=np.uint8).reshape(5, 5, 3)
arr[4, 4, :] = 255
data = xr.DataArray(arr.copy(), dims=['y', 'x', 'bands'],
coords={'bands': ['R', 'G', 'B']})
img = xrimage.XRImage(data)
print("img", img.data.values.min(), img.data.values.max())
img.stretch_linear()
print("stretched img", img.data.values.min(), img.data.values.max()) With tag Trollimage
and with
I'll create an issue on Trollimage. |
The integer stretching is fixed in pytroll/trollimage#153 . So we need a patch release of Trollimage to resolve this. |
We will do a patch release as soon as the PR is merged. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2640 +/- ##
========================================
Coverage 95.20% 95.21%
========================================
Files 354 356 +2
Lines 51393 51603 +210
========================================
+ Hits 48930 49132 +202
- Misses 2463 2471 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Pull Request Test Coverage Report for Build 6971000025Warning: This coverage report may be inaccurate.We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
💛 - Coveralls |
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 looks good to me. I'm not sure I followed the discussion over the last week or two on slack and on github but the final result looks reasonable.
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.
LGTM
This PR is a start to keep the
dtype
of the input data until saving. To have the tests pass, another PR is needed for Trollimage so that the enhancements don't upscale the data.The tests require pytroll/trollimage#150 to be merged (done) and a new Trollimage release including it. This is a side-effect of the Satpy feature applying a linear stretch by default, and in the current release this up-casts
float32
tofloat64
.AUTHORS.md
if not there already