Add reader for FY-4B / GHI and split AGRI reader into agri_fy4a_l1/agri_fy4b_l1

[simonrp84]

This PR adds support for the Geostationary High-speed Imager aboard FengYun-4B. Overall it's quite similar to the existing AGRI reader but has some specific modifications to deal with the geolocation for GHI (which only samples a small portion of the full disk).

• Tests added
• Fully documented

[codecov]

Codecov Report

Merging #2125 (6cdcb51) into main (fcbc517) will increase coverage by 0.05%.
The diff coverage is 99.60%.

@@            Coverage Diff             @@
##             main    #2125      +/-   ##
==========================================
+ Coverage   94.25%   94.30%   +0.05%     
==========================================
  Files         300      306       +6     
  Lines       45638    46039     +401     
==========================================
+ Hits        43014    43416     +402     
+ Misses       2624     2623       -1     

Flag	Coverage Δ
behaviourtests	4.62% <0.00%> (-0.04%)	⬇️
unittests	94.94% <99.60%> (+0.04%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
satpy/readers/fy4_base.py	98.61% <98.61%> (ø)
satpy/composites/agri.py	100.00% <100.00%> (ø)
satpy/readers/agri_l1.py	100.00% <100.00%> (+1.43%)	⬆️
satpy/readers/ghi_l1.py	100.00% <100.00%> (ø)
satpy/tests/compositor_tests/test_agri.py	100.00% <100.00%> (ø)
satpy/tests/reader_tests/test_agri_l1.py	100.00% <100.00%> (ø)
satpy/tests/reader_tests/test_fy4_base.py	100.00% <100.00%> (ø)
satpy/tests/reader_tests/test_ghi_l1.py	100.00% <100.00%> (ø)
satpy/tests/modifier_tests/test_angles.py	100.00% <0.00%> (ø)
satpy/tests/reader_tests/test_hy2_scat_l2b_h5.py	100.00% <0.00%> (ø)
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[simonrp84]

pre-commit.ci autofix

[coveralls]

Coverage increased (+0.05%) to 94.891% when pulling 6cdcb51 on simonrp84:fy4b_ghi into fcbc517 on pytroll:main.

[simonrp84]

pre-commit.ci autofix

[djhoese]

We talked about the AGRI readers in the community meeting today and some good ideas were discussed that I'll try to summarize here. Unfortunately they mean more work.

1. It was agreed that the sensor name returned here should include some platform-specific information or a version number. Others had similar concerns as me regarding the same instrument specification being used in future satellites so being able to reuse the same sensor name for multiple platforms would be nice (agri-fy4a versus agri-1 versus agri-a versus something else). @mraspaud thought it would be a good idea to reach out to some folks at CMA to see if they could shed some light on their future plans for AGRI on future FY-4 satellites. AGRI lists AGRI for all future FY-4 satellites so knowing what channels might be included, resolutions of those channels, and other related changes would be good to know so we don't have to change this reader in the future. If they think that each one will be significantly different from the previous satellite then having a platform name in the sensor name may make the most sense.
2. We would like to have AGRI as one reader called agri_l1 that could produce data from the FY-4A instrument or FY-4B instrument. The way we think this could be done it to leave out all the "wavelength" metadata in the YAML files and anything else that makes sense, put it in (hard code) the python code and use available_datasets functionality to handle determining what file is being read and what datasets are available. See https://satpy.readthedocs.io/en/latest/dev_guide/custom_reader.html#dynamic-dataset-configuration for more information. The basic idea would be that by implementing available_datasets, C10 is either the fy-4a version or the fy-4b version. Having a single reader is a major benefit for the user who only needs to use Scene(reader='agri_l1', ...), but puts all the burden on the developer/maintainers.

We had discussed other options like adding suffixes to the channel names to distinguish 4A from 4B channels, but decided this is too ugly, confusing, and overall unhelpful to the user.

New idea: If you wanted to the information to be in the YAML, it may be possible to combine the ideas of adding suffixes to the name (for the YAML) and the available_datasets method where you would remove the suffix and provide only the dataset names for the current sensor as being available. This saves us from having hardcoded wavelengths and metadata in the python code. It has the downside of having different names in the YAML than what is available when actually using the reader.

[djhoese]

As mentioned on slack, after talking with @simonrp84 and Kathy Strabala I'm starting to think that it would be better to have two readers as @simonrp84 originally had it. Simon pointed out that a user doing:

scn = Scene(reader='agri_l1', filenames...)
scn.load(["C10"])

and having to know what "C10" they were requesting is too confusing. It should be more explicit than that. Despite what we would have liked as far as instrument naming and design (not changing channel properties for the same channel name), we have what we have and have to live with it. I think being more explicit with two separate readers (with names that match the reader naming conventions) is best. @mraspaud rebuttal?

[mraspaud]

I'm not so much against it actually. It's unfortunate, but maybe it's for the best. As long as the file patterns are clearly different to allow find_fles_and_readers to do its job properly, then I'm willing to see what you come up with :)

[mraspaud]

Any news on this?

[djhoese]

There is a lot here so I'm not going to pretend I read all of it, but I had a few very small requests.

[simonrp84]

Thanks for the comments @djhoese. There certainly is a lot in here - turned into a far bigger job than expected.

Have updated the geolocation and the tests and have confirmed accuracy by comparing output images to coastlines, so hopefully all in order. I know the separation of AGRI on FY4A and FY4B isn't ideal, but can't think of a better solution.

[mraspaud]

Thanks for adding these readers! I just have a few comments, but otherwise this looks good.

[mraspaud]

I understand you have looked at the geometric accuracy of these. Have they been checked against both satellites and multiple repeat cycles?

[simonrp84]

I did, but having looked more closely I see some geolocation issues that I can't pin down. In addition, the different resolutions of AGRI data have different area extents (by several hundred meters) and I can't track that down either.

[mraspaud]

I can't see any difference between fy4a and fy4b extents, should they be factorised?

[simonrp84]

I haven't finished this, but have run out of time to work on it - can't guarantee I'll be able to come back to it for some time so hopefully someone else can pick it up. Am happy to supply data.

GHI seems pretty much OK. AGRI is causing issues as:

1. Different channels have different area extents, which is causing the native resampler to fail if reduce_data=True
2. The areas don't quite match coastlines, particularly for FY-4A/AGRI. I'm unsure if this is due to poor L1 data, poor area definition or both.

[simonrp84]

FWIW, my feeling is to edit the LOFF/COFF values supplied by CMA as I don't trust them. If we make those factors of each other (they're presently not) then the native resampler problem at least should go away and, hopefully, it would also improve geolocation.

[mraspaud]

LGTM