Refactor code for Sun zenith angle corrections and change to effective_solar_pathlength_corrected instead of sunz_corrected for built-in RGB recipes - #3397
Conversation
…ation and logging.
… ffunctionality to cap/reduce the correction.
…s already been applied.
I think in general I'm not against this PR "in spirit". The one thing I had trouble with when trying to switch to effective path length as described in your point above is that for some readers the However, I think this means that for VIIRS composites we'd have to redefine the |
|
You could always reverse the |
|
🤔 I'd prefer not to. |
|
@djhoese I understand the concern, but if I understand the comment correctly I don't think the proposed changes would be a problem and no significant change to the current behavior. Now the default
Does that make sense and sound ok for you you, or did I miss something else? I'll try to implement my suggested changes today such that it's more clear what I propose, and then we can see i the review process if this is acceptable or if I should revert something. |
|
What you say makes sense but I can't tell if you're saying we would still need to do what I mentioned. That is, VIIRS composites would need to use "sunz_corrected" for RGBs but either have to:
|
- Fix bug to make sure that custom sza values are used when actually intended and pass as optional_dataset instead of as a projectable. - Use different sza values in order to be able to properly test the available options concerning correction_limit and max_sza. - Add tests for more combinations of correction_limit and max_sza
…eSolarPathLengthCorrector
…sed as expected. Fix expected values to pass updated values of sunz_sza.
…y arrays instead. Also implement the same for the atmospheric_path_length_correction method
|
@djhoese Thanks a lot for the review and your comments! I have addressed them all now |
Co-authored-by: David Hoese <david.hoese@ssec.wisc.edu>
Co-authored-by: David Hoese <david.hoese@ssec.wisc.edu>
| if max_sza <= correction_limit: | ||
| raise ValueError( | ||
| "`max_sza` must be larger than `correction_limit` for a gradual " | ||
| "reduction of the correction to work.") |
There was a problem hiding this comment.
Is it possible to move this check to where the map_blocks call is so the error happens immediately rather than at compute time?
There was a problem hiding this comment.
Oh but also, these debug messages (logger.debug) will show up for every chunk that gets processed 🤔 What's the best way to do this...
There was a problem hiding this comment.
I mean...would it be terrible to move the logging and error of the functions up to just before the map_blocks call? Oh and there's the debug message for the else: case below.
There was a problem hiding this comment.
uups.. lesson learned not to put log messages in map_blocks, quite some spamming in the debug output indeed.
I moved the log messages and the ValueError to the parent method where I now also define the method and pass that as an argument to the map_blocks call - I think this looks cleaner overall. I still had to keep a check for a possible ValuesError in _sunzen_corr_cos_ndarray to satisfy mypy, but given how sunzen_corr_cos is defined, we would never reach that ValuesError.
I also moved the log message in _atmospheric_path_length_correction_ndarray for the same reason.
| def _atmospheric_path_length_correction_ndarray(data: np.ndarray, | ||
| cos_zen: np.ndarray) -> np.ndarray: |
There was a problem hiding this comment.
Sorry this is turning into such a headache, but Claude points out that the dtype in the map_blocks call is actually wrong. Rather than summarize what it said, here is the exact content:
_atmospheric_path_length_correction_ndarray (and _sunzen_reduction_ndarray) can return a
dtype other than the one declared to map_blocks. _sunzen_corr_cos_ndarray casts
(angles.py:628), but the other two do not. map_blocks now declares dtype=data.dtype /
meta=np.array((), dtype=data.dtype), so if cos_zen is float64 while data is float32 the dask
array claims float32 and produces float64 chunks. Verified locally:
declared float32, actual float64
That combination is reachable: get_cos_sza() casts lons/lats to the data dtype, but the
SZA-provided branch does coszen = np.cos(np.deg2rad(projectables[1])) with whatever dtype the
reader gave. The tests never hit it because call_sunz_modifier casts both arrays to the same
dtype. One-line fix in each: corr = corr.astype(data.dtype, copy=False) before the multiply, and
ideally a test with mismatched data/SZA dtypes.
There was a problem hiding this comment.
good catch. I just added one more level of parameterization to test different dtype combinations for data_arr and sunz_sza making sure that the dtype of data_arr is preserved . As expected this lead to test failures for _atmospheric_path_length_correction_ndarray and _sunzen_reduction_ndarray, but after adding corr = corr.astype(data.dtype, copy=False) to these methods, all tests now pass.
I did keep the dtype=dtype in the map_blocks call, which I think should be the case now when the dtype issue is fixed.
There was a problem hiding this comment.
You should be able to check this in the test and is something we have in tests. You can check the .dtype of the dask array returned and the .dtype of the computed numpy array created from that dask array. If they are the same and are np.float32 then 🎉
Co-authored-by: David Hoese <david.hoese@ssec.wisc.edu>
Co-authored-by: David Hoese <david.hoese@ssec.wisc.edu>
…ub.com:strandgren/satpy into refactor_solar_zenith_angle_correction_methods
… Add tests to make sure this is the case.
|
I tried to satisfy codescene, but for the remaining complaints I don't really agree:
|
|
I think Now if other things are all cleaned up maybe I should get to reviewing the overall idea again. What is the last thing we decided on VIIRS? Wait on the |
| if correction_limit is None or max_sza is None: | ||
| raise ValueError( | ||
| "Both `correction_limit` and `max_sza` are required for gradually " | ||
| "reducing the correction at large solar zenith angles.") |
There was a problem hiding this comment.
You mentioned in the other thread that this couldn't be moved. What if as a first try to make mypy happy you:
- Moved this to the caller.
- Change
max_szahere to not be optional. - In the method
ifstatements set correction_limit to 0.0 and max_sza to 0.0 if they areNone. Oh! Or set them toNaN.
Is that going too far? Either way I think correction_limits type annotation here is wrong since it says float and no optional.
This PR aims to clarify the code used for the Sun zenith angle corrections in satpy, including improving documentation and log messages to make it more transparent to the user. Furthermore, the code is prepared to eventually change the default behavior of the
SunZenithCorrector/sunz_correctedmodifier to remove the default reduction at high angles in order to compute the true reflectance. This change is planned in Satpy v1.0 and can be done by changing the new configuration parameter introduced in_config.pyhere to"use_legacy_sunz_correction": False. Finally, the built-in recipes are modified to improve imagery and comply with the recommendation from the WMO RGB workshop in Norrkoeping in April 2025.Background
In satpy we have two corrections for the Solar zenith angle/Solar path length available, the standard
SunZenithCorrectorwhich applies the1/cos(sunz)correction and theEffectiveSolarPathLengthCorrectorwhich applies the parameterization proposed by Li and Shibata (2006).There is also a default reduction of the correction starting at a given Sun zenith angle (88 degrees). This is intended for (RGB) imagery to avoid overcorrection at very large angles. However, this is not desirable for quantitative or scientific use of the data, where the standard
1/cos(sunz)should be used as is in order to compute the true reflectance. This functionality for capping/reduction has also been implemented in theEffectiveSolarPathLengthCorrector(duplicate code), but as shown in #3096 this reduction doesn't have any added value for the Li and Shibata parameterization, which works best as is, since it already accounts for the overcorrection at higher angles. For this reason, it was also agreed at the WMO RGB workshop in Spring 2025, that the Li and Shibata parameterization should be the recommended method for normalizing the data for the Solar zenith angle (without any capping or reduction).Furthermore, some datasets come with the
1/cos(sunz)already applied, in which case thesunz_correctedmodifier is attached to the dataset. However, this is not accurate since the default satpysunz_correctedmodifier is not only the simple1/cos(sunz), but also includes the reduction of the correction at higher Solar zenith angels. Therefore it would also be good if the defaultsunz_correctedwould rather be the simple1/cos(sunz)corrected, which would then be in-line with these datasets and also allow users in general to compute the true reflectance - this becomes even more relevant with #3292 where we clearly separate between "reflectance" data normalized by the Solar zenith angel and not.Changes in this PR
In response to the points above this PR includes the following changes:
Python code:
EffectiveSolarPathLengthCorrectormodifier. The parameters for this are still kept as valid input to avoid run-time errors if provided, but have no effect. A warning is issues if provided.EffectiveSolarPathLengthCorrectorto the same place as the other Solar zenith angle correction methods (./satpy/satpy/modifiers/angles.py)SunZenithCorrectorand underlying methods to make the functionality more clear.Change the default behavior ofKept as is for now, to be changed for Satpy v1.0.SunZenithCorrectorto compute the true reflectance without any reduction of the the correction.use_legacy_sunz_correctionwhich can be used to control whether to use the current legacy behavior ofSunZenithCorrector(with reduction) or not (simple1/cos(sunz)correction) if no reduction limits are explicitly defined in the modifier yaml configuration. For now this is set toTrueand should be changed toFalsein Satpy v1.0. Corresponding RST documentation has been added.SunZenithCorrectorandEffectiveSolarPathLengthCorrector, also highlighting the differences between the two and clarify that the former can be used to compute the true reflectance, whereas the latter is tailored for imagery.map_blocksyaml-recipes
effective_solar_pathlength_correctedmodifier instead ofsunz_correctedfor the solar zenith angle correction. As demonstrated in Standardize the correction for atmospheric path length for solar channels #3096 this leads to better imagery and was also suggested as recommendation at WMO RGB workshop in spring 2025.effective_solar_pathlength_correctedmodifier instead ofsunz_correctedfor the solar zenith angle correction of the red band used for reduced correction over clouds. This is done sincesunz_correctedis changing to become the simple 1/cos(sunz) correction which would not work well for imagery purposes since the reflectance of the red band would become 0 for angles larger than 90 degrees and thus break the Rayleigh correction. The Rayleigh correction reduction over clouds would also not work well close to 90 degrees, since also clear-sky becomes very bright at high sun zenith angles with the1/cos(sunz)correction.sunz_reducedmodifier for FCItrue_color,cloud_phaseandcloud_typeRGBs to be in-line with other sensors and recommendation from RGB workshop. Additionaltrue_color_sunz_reducedcomposite is added to still support the generation of this RGB with a smoother transition into deep-space. There may be some further changes/revisions to this since I plan some further optimization of the imagery with focus on the twilight area and blending with night-time imagery in a later PR once this one is approved and merged.Special cases
Some readers provide the reflectance data with the
1/cos(sunz)correction already applied, in which case thesunz_correctedmodifier is already attached to the dataset. This is the case for the following readers:For
meris_nc_sen3,olci_l2andclavrxthere are no pre-configured RGB recipes, so the changes in this PR are not relevant for these data. Formsi_safe_l2athe data have the dedicated modifieresa_sunz_correctedattached to them, which is also used for the corresponding RGB recipes. Hence, the changes in the PR are not relevant for these data either.The only relevant one is VIIRS (SDR). Since the VIIRS SDR data come with the Solar zenith angle correction already applied, the dataset gets the
sunz_correctedmodifier attached. However, VIIRS L1B data come without the1/cos(sunz)correction and therefore nosunz_correctedmodifier is attached to the dataset.For this reason, we can use theEffectiveSolarPathLengthCorrectormodifier for the recipes, but we have to name itsunz_corrected, in order for satpy to understand that this is equivalent to the sunz correction already in the SDR data in the sense that no further correction should be applied.A better and more generic solution would be to modify the dependency tree to know that if the user requests a dataset with theeffective_solar_pathlength_correctedmodifier, but only finds a DataID with the modifiersunz_corrected, it would be regarded as equivalent and a substitute for theeffective_solar_pathlength_correctedmodifier. However, such a solution is outside the scope of this PR.After the monthly meeting on June 2, it was agreed to use the regular
effective_solar_pathlength_correctedmodifier also for VIIRS data. Currently this won't work with VIIRS SDR data, since satpy will fail to find the "base" version of the requested without modifiers. It was agreed that this should be solved by adding the unnormalized_reflectance calibration level to theviirs_sdrreader once #3292 is merged, see details in #3411. We then just need to make sure that the VIIRS composites specify the expected calibration level in the composite and/or modifier recipes.Changes for users and backwards incompatibility
sunz_correctedmodifier at 88 degrees. See Standardize the correction for atmospheric path length for solar channels #3096 and comments in the PR for examples.sunz_reducedmodifier leads to significantly more data being retained/visible close to the terminator. see examples below.effective_solar_pathlength_correctedmodifier has been removed as it's not needed, given that the Li and Shibata parameterization already deals with the over-correction at high solar zenith angels. If the reduction parameters are still provided they will be accepted but ignored and a warning is raised.effective_solar_pathlength_correctedmodifier will not be possible with VIIRS SDR data until Addunnormalized_reflectanceto VIIRS SDR (viirs_sdr) reader #3411 has been fixed.The default reduction of thesunz_correctedhas been disabled (but still with support to apply it if needed). Hence, thesunz_correctedmodifier will compute the true reflectance if applied with it's new default configuration. This will impact local composites which use the upstreamsunz_correctedmodifiers defined in satpy (except for VIIRS which is treated differently given the differences between the L1B and SDR data). The result will be very bright imagery close to 90 degrees and no data beyond 90 degrees. A warning has been added when using these modifiers to inform users on this change and how to fix it.Similarly, if users have defined there own localsunz_correctedmodifier with default configuration (i.e. nocorrection_limitormax_sza), the imagery will change close to the terminator (again becoming very bright close to 90 degrees and black beyond).Following the monthly meeting on June 2, this PR has been changed such that there are no changes to the default values in
SunZenithCorrectoror corresponding pre-configuredsunz_correctedmodifiers in satpy at this point. Instead we will change this in v1.0 (#3412) and a warning is therefore now raised about this upcoming change of the default values ofSunZenithCorrectorare used. A new configuration parametersuse_legacy_sunz_correctionhas been added to control whether to use the legacy correction (with reduction) or not. Currently it's set toTrue, but should be changed toFalsein Satpy v1.0AI was used to support this PR, mainly for refactoring tests and writing doc-strings