Skip to content
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

New reader for Himawari L2 NOAA enterprise cloud products. #2558

Merged
merged 19 commits into from
Oct 11, 2023

Conversation

simonrp84
Copy link
Member

@simonrp84 simonrp84 commented Aug 24, 2023

This PR adds a reader for the L2 cloud products products by NOAA for the Himawari satellites. These products are distributed via NOAA's big data programme in netCDF4 format and can be found at, for example: AHI-L2-FLDK-Clouds.

  • Add support for all cloud mask datasets
  • Add support for all cloud phase datasets
  • Add support for all cloud height datasets
  • Tests added
  • Fully documented

@simonrp84
Copy link
Member Author

pre-commit.ci autofix

@codecov
Copy link

codecov bot commented Aug 24, 2023

Codecov Report

Merging #2558 (8cbe04a) into main (f018f12) will increase coverage by 0.01%.
Report is 19 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2558      +/-   ##
==========================================
+ Coverage   94.92%   94.93%   +0.01%     
==========================================
  Files         352      354       +2     
  Lines       51225    51401     +176     
==========================================
+ Hits        48624    48800     +176     
  Misses       2601     2601              
Flag Coverage Δ
behaviourtests 4.27% <0.00%> (-0.01%) ⬇️
unittests 95.55% <100.00%> (+0.01%) ⬆️

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

Files Coverage Δ
satpy/readers/ahi_l2_nc.py 100.00% <100.00%> (ø)
satpy/tests/reader_tests/test_ahi_l2_nc.py 100.00% <100.00%> (ø)

... and 6 files with indirect coverage changes

@djhoese
Copy link
Member

djhoese commented Aug 24, 2023

How do these differ from ABI L2? Should we share the python code?

@simonrp84
Copy link
Member Author

I think the ABI ones contain enough metadata to create an area Def while the AHI ones do not (just lat lon values). Aside from that, they're broadly similar.

@coveralls
Copy link

coveralls commented Aug 25, 2023

Pull Request Test Coverage Report for Build 5977747983

  • 121 of 121 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 95.481%

Totals Coverage Status
Change from base Build 5939673825: 0.01%
Covered Lines: 48792
Relevant Lines: 51101

💛 - Coveralls

@simonrp84 simonrp84 marked this pull request as ready for review August 25, 2023 11:31
@simonrp84 simonrp84 added the enhancement code enhancements, features, improvements label Aug 25, 2023
@djhoese
Copy link
Member

djhoese commented Aug 31, 2023

Instead of defining every possible L2 product in the YAML, could we dynamically load these and use the variable name from the file?


aex = get_area_extent(pdict)

pdict['a_name'] = 'Himawari_Area'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the HSD reader we use self.basic_info["observation_area"] for the name of the area. Is there anything like that in these files? Or if not in these files, what does observation area equal in the HSD files?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, not really. The self.nc.attrs['cdm_data_type'] attrs stores an area name but it's always Full Disk, which isn't very meaningful. I can set pdict['a_name'] to that if you prefer though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's not an in-file attribute for this, then can we guess a decent abbreviation/region name based on the filename?

@simonrp84
Copy link
Member Author

Instead of defining every possible L2 product in the YAML, could we dynamically load these and use the variable name from the file?

We could but I quite like having them explicitly defined as it makes it easier to browse what products can be loaded before you even download the files. If you have strong feelings about it then I can change though.

@djhoese
Copy link
Member

djhoese commented Aug 31, 2023

With other L2/EDR readers we (I) have moved to dynamic loading. I agree it is nice to be able to look at the YAML and say "yep, it's called that and it's supported", but I would argue that you can do the same with dynamic variables by looking at the YAML file patterns "yep, this file pattern is listed and the variable I want is 2D, so it must be supported".

I'd rather not have to maintain a potentially long list of variables in the YAML and update every time a new L2 product comes out. Additionally, for polar-orbiters the Longitude/Latitude handling gets really sketchy (not that that's an issue for this reader).

I can be convinced otherwise...but not as easily as I have been on other issues since I'd likely be the person to add new products.

@simonrp84
Copy link
Member Author

Can you point me at an example of a dynamically loaded YAML? Happy to change, but am unfamiliar with the syntax for this...

@djhoese
Copy link
Member

djhoese commented Aug 31, 2023

The YAML isn't dynamic, the python code is. Any reader that defines the available_datasets method is doing this:

def available_datasets(self, configured_datasets=None):

That includes my recently merged (and updated version of your) VIIRS EDR reader. That one got a little complicated due to the different versions of lon/lat variables in each file.

@djhoese
Copy link
Member

djhoese commented Oct 9, 2023

Regardless of your feelings about the dynamic dataset definitions, the tests are failing and will need to be fixed before merging.

@simonrp84
Copy link
Member Author

Looks like the only remaining failing test is for the CF writer code rather than anything this PR has touched.

Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the reader! Just a few comments inline, but looks good otherwise.

satpy/readers/ahi_l2_nc.py Outdated Show resolved Hide resolved
satpy/readers/ahi_l2_nc.py Show resolved Hide resolved
satpy/readers/ahi_l2_nc.py Outdated Show resolved Hide resolved
satpy/readers/ahi_l2_nc.py Outdated Show resolved Hide resolved
satpy/readers/ahi_l2_nc.py Outdated Show resolved Hide resolved
@simonrp84
Copy link
Member Author

Have adopted all your suggestions, @mraspaud!

There was previously a test to ensure that the warning was raised about the full disk area. As this is now an info message I'm not testing it but can add a test if it really needs one..

@mraspaud mraspaud added this to the v0.44.0 milestone Oct 11, 2023
Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@mraspaud mraspaud merged commit e804f2f into pytroll:main Oct 11, 2023
19 of 20 checks passed
@simonrp84 simonrp84 deleted the ahi_l2_noaa branch October 11, 2023 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:readers enhancement code enhancements, features, improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants