-
Notifications
You must be signed in to change notification settings - Fork 298
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
Correct Unexpected floats when reading LI L2 LFL #2998
base: main
Are you sure you want to change the base?
Conversation
…he _FillValue is not into the attributes
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2998 +/- ##
=======================================
Coverage 96.08% 96.08%
=======================================
Files 377 377
Lines 55150 55155 +5
=======================================
+ Hits 52992 52997 +5
Misses 2158 2158
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
Pull Request Test Coverage Report for Build 11970823559Details
💛 - 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.
Thanks for the investigations! I left some comments in the code.
I am also wondering if we still also need the fix for the float64
upcasting when multiplying with the scale_factor
, as discussed in the issue...
@@ -634,9 +632,6 @@ def test_coords_generation(self, filetype_infos): | |||
elevation_vals = elevation.values * point_height | |||
azimuth_vals *= -1 | |||
lon_ref, lat_ref = projection(azimuth_vals, elevation_vals, inverse=True) | |||
# Convert to float32: | |||
lon_ref = lon_ref.astype(np.float32) | |||
lat_ref = lat_ref.astype(np.float32) |
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'm a bit confused here: why do we need to remove the casting to float32 in the test reference data? What is the type of the lon
and lat
coming from the reader code, after this PR? I would have expected that after this PR the values should be in float32 from both sources and the test should pass without modifications...
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.
@ameraner in fact lon
and lat
do not have have a _FillValue
attributes and it is natively a np.float64
or before my commit this line bellow was applied which converts the array from a np.float64 to a np.float32
arr = arr.where(arr != arr.attrs.get("_FillValue"), fill_value)
So it means that I have to investigate this method
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.
hmm ok, then could you please check where the lon
and lat
arrays become float64
in the code? As requested, part of the fix we need is to avoid float64
in favour of float32
for float values...
Makes sure that the method apply_fill_value is not applied when
_FillaValue
is not into the attributes. It is fixing the bug mentioned there #2854 .