Set dtype for get_lonlats() in NIR reflectance calculation #2642
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2642 +/- ##
==========================================
- Coverage 95.21% 95.21% -0.01%
==========================================
Files 356 356
Lines 51591 51588 -3
==========================================
- Hits 49120 49117 -3
Misses 2471 2471
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Hmm, maybe checking NIR |
Pull Request Test Coverage Report for Build 6966833397
💛 - Coveralls |
satpy/modifiers/spectral.py
Outdated
| @@ -107,7 +107,7 @@ def _get_sun_zenith_from_provided_data(projectables, optional_datasets): | |||
| if sun_zenith_angle is None: | |||
| raise ImportError("Module pyorbital.astronomy needed to compute sun zenith angles.") | |||
| _nir = projectables[0] | |||
| lons, lats = _nir.attrs["area"].get_lonlats(chunks=_nir.data.chunks) | |||
| lons, lats = _nir.attrs["area"].get_lonlats(chunks=_nir.data.chunks, dtype=_nir.dtype) | |||
There was a problem hiding this comment.
I don't understand what the dtype of nir has to do with the type of longitudes and latitudes, och sun angles? I don't think it's the role of this function to decide on the type of the sun zenith...
There was a problem hiding this comment.
The dtype of sun angles is determined by the lon and lat dtypes. If the dtype of sun angles is float64 it forces the 3.9 um reflectance to be float64 no matter what the dtype of the input data is. NIR is the input data, so using its dtype is plain logical, and to save everything in between to be up- and then later downcast the best way to is to get the lonlats with the same dtype. Lons, lats and SZA are just intermediate data in the NIR reflectance determination, so it this saves a lot to use the intended type from the beginning without side-effects.
There was a problem hiding this comment.
Ok, I understand now that we need the sunz to be the same type as nir for later computations. But still, it's really hard to understand without context why the dtype is set to nir's, and could be confusing in the future.
Looking up a bit, it looks like this function is called from _get_reflectance_as_dataarray right? That function is at a higher abstraction level, and I think there it would make sense to decide the type of the sunz. So can we just pass down the dtype from there to _get_sun_zenith_from_provided_data?
There was a problem hiding this comment.
also, if we're modifying this function, it seems we are passing projectables here, while we just use the first element of the list, can we change that to just pass nir to the method?
There was a problem hiding this comment.
Ok, those make sense. Updated in 4c7b330
satpy/modifiers/spectral.py
Outdated
| @@ -95,7 +95,7 @@ def _get_tb13_4_from_optionals(optional_datasets): | |||
| return tb13_4 | |||
|
|
|||
| @staticmethod | |||
| def _get_sun_zenith_from_provided_data(projectables, optional_datasets): | |||
| def _get_sun_zenith_from_provided_data(_nir, optional_datasets, dtype): | |||
There was a problem hiding this comment.
just nitpicking here, but I would rename _nirto nir here.
…nuu/satpy into preserve-nir-reflectance-dtype
|
I'm merging this now, thanks for the hard work! |
This simple addition finalizes pytroll/pyspectral#206 so that
NIRReflectancemodifier doesn't up-cast the input data tofloat64.Includes also some refactoring between
NIRReflectanceandNIREmissivePartFromReflectance.AUTHORS.mdif not there already