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
Add NDVI-scaled hybrid green correction #2280
Add NDVI-scaled hybrid green correction #2280
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2280 +/- ##
==========================================
+ Coverage 94.32% 94.36% +0.04%
==========================================
Files 308 311 +3
Lines 46168 46703 +535
==========================================
+ Hits 43546 44073 +527
- Misses 2622 2630 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
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.
Nice! Glad to see this composite added :)
I have a couple of comments in the docs.
If you have the DOI for the paper this is based on I think you can add:
in the docstring and our custom sphinx DOI plugin will link it to the paper. |
It should be noted that the examples shown above are based on a first guess of the tunable parameters of the corrector. I suggest to keep it like this for the moment and then we can update it later when we have performed a more quantitative validation. Question: Do you think this is mature enough to replace the current hybrid green method in the AHI true color RGBs, or shall we rather keep the current correction for now? Either way I would include both, but make one default e.g. |
With the last commits I have refactored the code used for the green band corrections in order to make them more generalized and aligned with the description of the method in the literature. I also made the new NDVI weighted hybrid green correction the default one for FCI true color imagery, for now using preliminary limits of the blending factors which will be tuned once we get real data. |
Thanks, I added this in one of the later commits. |
@simonrp84 I cannot find any comments. I'm a looking at the wrong place? Please not that I have done some more work on this today, including some refactoring. I also added some test results in the comments above :) |
satpy/composites/spectral.py
Outdated
modifiers: [sunz_corrected, rayleigh_corrected] | ||
standard_name: toa_bidirectional_reflectance | ||
|
||
In this example, pixels with NDVI=0.0 (default `ndvi_min`) will be a weighted average with 85% contribution from the |
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.
Is this correct? I thought that for very green objects we wanted more of the 0.8 channel, not less - as we miss the chlorophyll peak in the FCI/AHI 0.5 micron band we need to boost it.
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.
Well this is the interesting bit with the green band correction. The location of the AHI/FCI green bands has two effects:
- Vegetation appears brownish.
- Barren appears reddish.
The green color of vegetation can be boosted with a higher contribution from the NIR channel. Similarly, the reddishness of barren is reduced with an increasing contribution from the NIR channel. However, more contribution from NIR is required to get the proper color of barren, compared to vegetation. Hence, I came up with the solution of increasing the contribution from NIR with decreasing NDVI.
Below are a few examples using the default hybrid green method, with decreasing contribution from the NIR channel. As you can see the color of Australia starts to get too red when the color of vegetation start to look realistic. I also included the result of the NDVIHybridGreen (with just qualitative tuning) and VIIRS RGB from NASA worldview as reference.
15% NIR | 10% NIR | 7% NIR | 4% NIR | 0% NIR | With this PR | VIIRS from NASA Worldview |
---|---|---|---|---|---|---|
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.
Hence, I came up with the solution of increasing the contribution from NIR with decreasing NDVI.
Your solution, does this mean that this is just for FCI so is "new" or is this being applied to AHI by some other group and documented in some paper?
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 is only internal developments in preparation for FCI. We are testing with AHI since we have the same scenario with sub-optimal position of the green band there. Hopefully we can publish it at some point though.
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.
How much of this is backwards compatible with what existed before? I see classes changing names, composites changing names, etc. I will need to make copies of the originals for my own application if these are merged so I just want to make sure.
@djhoese If I implemented everything according to plan, everything should be backwards compatible. The renaming of classes and composites is done to make them more generic (they started off as Himawari-specific) and also align the naming with the corresponding literature. One aspect that would change is the green correction of AHI true color RGBs, since I changed the value to be in-line with the paper on which the correction is based. I can, of course revert this, if preferred. See my comment above on this topic. |
… if importing 'GreenCorrector' from this file. Will be covered by deprecated warning.
I had underestimated to which extent this PR could potentially cause compatibility problems for other users. After discussion with @mraspaud I have re-introduced the old class ( Finally I also changed the default fractions for the hybrid green band correction of the true color RGBs for AHI/AMI, such that they are in line with the current main (rather than on the paper on which they're based). |
The documentation build fails with:
|
@pnuu Thanks I missed that. I cannot directly see what is the issue though, at least not for the warning. Can I see what test that is failing in order to test solutions offline? |
I think doing |
satpy/composites/spectral.py
Outdated
For example, to simulate an FCI corrected green composite, one could use | ||
a combination of 93% from the green band (vis_05) and 7% from the | ||
near-infrared 0.8 µm band (vis_08):: | ||
hybrid_green = (1 - F) * R(0.51) + F * R(0.86) |
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.
Maybe try making this a literal block (indent and put ::
on the line above) and see if that fixes the one error.
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, that was the issue. Now it seems to work.
satpy/composites/spectral.py
Outdated
ndvi_hybrid_green: | ||
compositor: !!python/name:satpy.composites.spectral.NDVIHybridGreen | ||
limits: [0.15, 0.05] | ||
prerequisites: | ||
- name: vis_05 | ||
modifiers: [sunz_corrected, rayleigh_corrected] | ||
- name: vis_06 | ||
modifiers: [sunz_corrected, rayleigh_corrected] | ||
- name: vis_08 | ||
modifiers: [sunz_corrected ] | ||
standard_name: toa_bidirectional_reflectance |
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 think this whole thin should be indented one level and ::
put on the previous line to make it literal.
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, that was the issue. Now it seems to work.
as @ameraner learned last week, the line number in the errors refers to the line number from the start of the docstring (ie not from the beginning of the file) |
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.
Few typos and inconsistent syntax, otherwise LGTM.
Co-authored-by: Panu Lahtinen <pnuu+git@iki.fi>
Co-authored-by: Panu Lahtinen <pnuu+git@iki.fi>
Co-authored-by: Panu Lahtinen <pnuu+git@iki.fi>
Thanks for spotting @pnuu , I should consider re-activating the PyCharm spell-check.. |
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.
Just a couple of comments, otherwise LGTM
Co-authored-by: Martin Raspaud <martin.raspaud@smhi.se>
…com/strandgren/satpy into feature-ndvi_corrected_hybrid_green
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, thanks for adding this great correction to the green channel!
satpy/etc/composites/ahi.yaml
Outdated
- wavelength: 0.51 | ||
modifiers: [sunz_corrected, rayleigh_corrected] | ||
- wavelength: 0.85 | ||
modifiers: sunz_corrected] |
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.
Something is not being tested. This syntax is not correct.
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.
Indeed this syntax leads to an error, with sunz_corrected]
getting split into single characters:
[DEBUG: 2022-12-06 16:01:05 : satpy.node] Skipping optional DataQuery(name='solar_zenith_angle', calibration='reflectance'): Unknown dataset DataQuery(name='solar_zenith_angle', calibration='reflectance')
Traceback (most recent call last):
File "/tcenas/home/strandgren/git/ext/py/satpy/latest/satpy/satpy/scene.py", line 1362, in _update_dependency_tree
self._dependency_tree.populate_with_keys(needed_datasets, query)
File "/tcenas/home/strandgren/git/ext/py/satpy/latest/satpy/satpy/dependency_tree.py", line 262, in populate_with_keys
raise MissingDependencies(unknown_datasets, "Unknown datasets:")
satpy.node.MissingDependencies: Unknown datasets: {DataQuery(name='green'): {DataQuery(wavelength=0.85, modifiers=('s', 'u', 'n', 'z', '_', 'c', 'o', 'r', 'r', 'e', 'c', 't', 'e', 'd', ']')): {DataQuery(wavelength=0.85, modifiers=('s',))}}}
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/tcenas/home/strandgren/git/ext/py/satpy/latest/satpy/satpy/scene.py", line 1364, in _update_dependency_tree
raise KeyError(str(err))
KeyError: "Unknown datasets: {DataQuery(name='green'): {DataQuery(wavelength=0.85, modifiers=('s', 'u', 'n', 'z', '_', 'c', 'o', 'r', 'r', 'e', 'c', 't', 'e', 'd', ']')): {DataQuery(wavelength=0.85, modifiers=('s',))}}}"
I have fixed this now, but I'm not sure what kind of (unit) test that would detect this kind of syntax? Did I miss something?
And regarding Polar2Grid you can wait with merging this if you want, no hurry from my side. Although, I have been very careful in making sure that it's fully backwards compatible, but I cannot not guarantee that I have not missed anything. For example the ratios of the green correction are not changing with this PR, the AHI true color images should look like before if the same composite names are used.
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.
And regarding Polar2Grid you can wait with merging this if you want, no hurry from my side. Although, I have been very careful in making sure that it's fully backwards compatible, but I cannot not guarantee that I have not missed anything. For example the ratios of the green correction are not changing with this PR, the AHI true color images should look like before if the same composite names are used.
I see that now. Thank you for working so hard to preserve backward compatibility not just in the Python code but the YAML configuration for the composites as well. If there is no hurry on this then I'd like to wait a day or two at least before merging so I can straighten things out in Geo2Grid before the version 1.1 release. I can always force the commit SHA that we use in our builds, but we are also like a day away from release so I'd like to not risk anything if possible. I have a meeting about Geo2Grid tomorrow so I'll know more then.
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 adds a new approach for correcting the reflectance from green channels that miss the chlorophyll reflectance peak at 0.55 microns, e.g. AHI and FCI.
The approach is based on the hybrid green method from Miller et al. (2016), but using dynamic blend factor that depends on the scene NDVI. The larger the NDVI, the smaller the contribution from the NIR (0.86 micron) channel will be.