Add get_area_def to cf reader

[BENR0]

This adds add area definition support for the cf reader. I fixed the tests and added a test area in geos projection as well as some preliminary assertions for testing the area.
I think the tests should be improved and extended to cover other areas. Currently the extent assertion is failing because of pytroll/pyresample#355.

• Closes Add AreaDefinition support to the 'satpy_cf_nc' reader #1672
• Tests added

[djhoese]

Pyresample 1.20. 0 is being deployed right now. It looks like our test environment uses pyresample from conda-forge so we'll have to wait for that package to be pull requested, merged, released, and synced to the package repository.

[mraspaud]

This is failing, so it'll have to go to the next release

[djhoese]

@zxdawn @BENR0 @mraspaud So I just ran this locally with pyresample main and it does actually fail even though the CI jobs here aren't pulling in the right version:

>           assert expected_area.area_extent == actual_area.area_extent
E           AssertionError: assert (339045.5577,... 4803645.4685) == (4803645.4685..., 339045.5577)
E             At index 0 diff: 339045.5577 != 4803645.468500001
E             Use -v to get the full diff

satpy/tests/reader_tests/test_satpy_cf_nc.py:144: AssertionError

Edit: Note this is way better than the results from the old pyresample version:

 >           assert expected_area.area_extent == actual_area.area_extent
E           AssertionError: assert (339045.5577,... 4803645.4685) == (171902444919...3027029152.95)
E             At index 0 diff: 339045.5577 != 171902444919656.84
E             Use -v to get the full diff

[BENR0]

It seems this was due to a switch of x and y in the test setup. Now it passes. One caveat though is that I had to add pytest.approx to the assert statement because the upper right y coordinate was just a tiny bit off from the expected value.

[djhoese]

You may need to merge main into this branch. It seems github actions doesn't like the old workflow file (I hope that's the problem at least).

[BENR0]

Indeed I totally forgot to do that.

[codecov]

Codecov Report

Merging #1695 (2b2048b) into main (ff54a74) will increase coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1695      +/-   ##
==========================================
+ Coverage   94.05%   94.13%   +0.08%     
==========================================
  Files         290      293       +3     
  Lines       44639    45079     +440     
==========================================
+ Hits        41987    42437     +450     
+ Misses       2652     2642      -10     

Flag	Coverage Δ
behaviourtests	4.68% <0.00%> (-0.04%)	⬇️
unittests	94.79% <100.00%> (+0.07%)	⬆️

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

Impacted Files	Coverage Δ
satpy/readers/satpy_cf_nc.py	97.97% <100.00%> (+0.17%)	⬆️
satpy/tests/reader_tests/test_satpy_cf_nc.py	100.00% <100.00%> (ø)
satpy/writers/geotiff.py	93.75% <0.00%> (ø)
satpy/tests/test_dataset.py	100.00% <0.00%> (ø)
satpy/readers/seviri_l1b_native_hdr.py	100.00% <0.00%> (ø)
satpy/tests/reader_tests/test_seviri_l1b_native.py	100.00% <0.00%> (ø)
satpy/readers/mws_l1b.py	98.51% <0.00%> (ø)
satpy/tests/reader_tests/test_mws_l1b_nc.py	100.00% <0.00%> (ø)
satpy/readers/pmw_channels_definitions.py	97.60% <0.00%> (ø)
satpy/tests/test_writers.py	98.99% <0.00%> (+0.02%)	⬆️
... 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.

[coveralls]

Coverage increased (+0.07%) to 94.74% when pulling 2b2048b on BENR0:feat_add_get_area_def_to_cf_reader into ff54a74 on pytroll:main.

[BENR0]

@djhoese @mraspaud did you have time to look at this?

[mraspaud]

@BENR0 sorry for the late reply.

It looks good for the area case, but what happens if we are working with swath definitions (so we just have lons and lats)? will the reading crash then?

[BENR0]

I think that depends if this is covered by the implementations in pyresample. I can have a look at it even though I am not that familiar with the code. Maybe @TomLav can help because most of the code is from him.

[mraspaud]

@BENR0 I just meant to verify that the changes you make do not change the behaviour of the lon/lat case, no need to implement it here. So if you can see that the lon/lat case still works as before, then this is good as it is now

[BENR0]

@mraspaud do I understand correctly that you mean if a netcdf written from a Satpy Scene which only has a swath definition is also read correctly?

[mraspaud]

I haven't checked recently, but I would expect so.

[mraspaud]

Although it might be the same problem as you are fixing here that the geolocation isn't actually read.

[TomLav]

Thanks for tagging me. I am maybe wondering if cf_reader should offer a richer interface to pyresample's from_cf() routine? For example, from_cf() offers the option to request the area_def attached to a specific netCDF variable which is not available from the cf_reader() at present.
To specify variable= in from_cf() can useful both if there are several area_defs in a netCDF file (not illegal, but not often) and if the CF encoding is not fully standard (in which case specifying variable= helps from_cf() to locate the area_def).
But I do not know if the cf_reader workflow only looks at file-level area_defs or variable-level.

[BENR0]

Indeed I think that would be something to think about to make it more generic in the future. Currently I think the reader is mainly for reading files written with Satpy which I think does not produce different area_defs in one file. So my opinion is to make this another PR (respectively create an feature issue for it).

[TomLav]

Thanks. Your approach is fine with me.

[BENR0]

@mraspaud regarding the swath definition case: You were right implementing the get_area_def the way I did breaks this case. The reason is that the CF utils in pyresample implemented by @TomLav do not cover that case. Without the get_area_def method implemented in the specific reader Satpy "falls back" to creating the area from the lat/lon grids which is implemented in the FileYAMLReader. For a quick work around I added ValueError to the try except in order to fall back to the yaml reader creating the area def. While this works I think this is basically asking for trouble since ValueError is pretty generic and might lead to more problems that it solves.

My opinion about this is that handling 2D lat/lon coordinate grids should also be implemented in the CF utils in pyresample because CF allows this kind of coordinates and therefore AreaDefinition.from_cf should be able to handle this.
Apart from this obviously there is a unit test missing in the cf reader for the 2D lat/lon grid case. Are there some helper functions somewhere already to create a Satpy dataset with a swath definition which could be reused?

[TomLav]

Hello. I think it is correct of pyresample's from_cf() to not support lat/lon swath data (if this is what was failing now). I think AreaDefinition is only for grids based on Earth projections, while the geometry information of lat/lon swath would use SwathDefinition object.

I don't know satpy. Can you link me to the current implementation of this feature in satpy, so that I can see how it is working?

[mraspaud]

sounds good, feel free to refactor!

[BENR0]

@mraspaud I refactored the dataset generation to be in the set up for all tests. I left in the writing of the header attribute instrument because #2176 is not merged yet and thus tests would fail.

[mraspaud]

#2176 is now merged @BENR0

[BENR0]

@mraspaud ok will update this pr later today or tomorrow morning.

[BENR0]

@mraspaud I merged main and refactored out the instrument lines in the tests.

[mraspaud]

@BENR0 regarding the failing tests on windows, I can recommend the usage of the tmp_path fixture, maybe that can help.

[mraspaud]

LGTM

[djhoese]

@BENR0 Any time to look into the failing tests?

[BENR0]

@djhoese yes I looked at it and it seems that the failing test on windows is due to the same filename being used in two tests. I don't know if @mraspaud hint with the tmp_path fixture can fix that. But it seems that in later tests file pathes are just counted up to make them different. I can just use that same technique here if that is ok with you.

[mraspaud]

Yes the fixture seems to work better on windows than the tempdir functions

[djhoese]

@BENR0 if a single test function is writing multiple files then counting and produce unique filenames seems reasonable. Although this is probably a sign that the function should be split into multiple tests.

Using tmp_path as the destination for the files you create in the tests should mean that each test gets its own unique directory and so one test shouldn't interfere with another.

[BENR0]

@djhoese @mraspaud since the tests are using unittest.testcase the pytest tmp_path fixture does not work. I actually think it would be nice to use it but that would need some refactoring. Haven't used the tmp_path fixture before so could be I am missing something and there is a workaround somehow.

Maybe I will have some time next week to work on this. If this should move forward faster I could just do the hacky filename numbering in the mean time.

[djhoese]

Give me 5 seconds. I'll fix it.

[djhoese]

@BENR0 I've made a PR to your fork that should do the pytest and tmp_path changes. It cleans up the code a ton:

BENR0#1

[djhoese]

Note I assume that if the filenames are in a separate tmp directory for each test that the filenames can be the same for each test. If that means the tests are no longer testing what they were before then we can make multiple fixtures for each filename format.

[BENR0]

:-D after I wrote my comment I started the same changes you made and tested it for the first test. But didn't have the time to finish it. Thanks for the work.

[djhoese]

Yeah, no problem. I'm avoiding my other work so this was a nice distraction and I've pytest'd quite a few test modules at this point. All you have to do now is merge the PR in your fork and the tests here should pass...I hope.

[mraspaud]

@djhoese @BENR0 nice with the switch to pytest!

[djhoese]

The unstable test failure seems to be a problem in rasterio...I hope. I'm going to try restarting the test and see what happens. If it fails again I think I'll merge this anyway and then work on figuring out what broke in a separate PR.

[djhoese]

Filed rasterio/rasterio#2591 with rasterio. Let's merge this and worry about the failing test somewhere else.