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

Fix warning when CF-writing a Scene with SwathDefinition area #2411

Merged
merged 4 commits into from Mar 14, 2023

Conversation

ghiggi
Copy link
Contributor

@ghiggi ghiggi commented Mar 13, 2023

This PR fixes an annoying erroneous warning occurring when writing with the CFWriter a satpy Scene to disk.

@ghiggi ghiggi requested a review from mraspaud as a code owner March 13, 2023 15:46
@djhoese
Copy link
Member

djhoese commented Mar 13, 2023

What is the end result of this change? Does/should a SwathDefinition get a grid mapping? Because it isn't a gridded dataset so does CF grid mapping data handle 2D lon/lat arrays as coordinates?

@ghiggi
Copy link
Contributor Author

ghiggi commented Mar 13, 2023

The end result of this PR is simply to just not raise a warning when the area is a SwathDefinition.
Regarding the other questions:

  • Yes, CF conventions say that when you have 2D lon/lat arrays you could/should also add a CRS coordinate for the geographic coordinates and add the grid_mapping: <crs_coord_name> and a coordinate: longitude latitude attribute to the data variables.

  • In the case you have projected data (and you also include the longitude/latitude as coordinates), you should/could attach two CRS coordinates to the dataset, and reference both with, for example, grid_mapping: "crsWGS84: longitude latitude goes_abi_1km x, y"

@djhoese
Copy link
Member

djhoese commented Mar 13, 2023

Note that there are some failing tests in all PRs due to an update in NetCDF and how we handle compression (apparently). @pnuu and @sfinkens were looking into it.

@ghiggi
Copy link
Contributor Author

ghiggi commented Mar 13, 2023

@djhoese I kinda just rushed on this because I would really like to have this warning removed with the next satpy release (which I plan to use to run the production of the dataset I am working on) to avoid my production log file and terminals be overwhelmed by this logger warning.
I modified the code to keep almost the current behavior and just do not print the warning if the area is a SwathDefinition.

@mraspaud
Copy link
Member

@ghiggi failing tests should now be fixed, feel free to merge main into your PR to fix CI

@ghiggi
Copy link
Contributor Author

ghiggi commented Mar 14, 2023

@mraspaud I further simplified the PR to just avoid issuing a warning when the area is a SwathDefinition.
I guess it's ready to be merged ... but let's wait the tests ... ;)

@codecov
Copy link

codecov bot commented Mar 14, 2023

Codecov Report

Merging #2411 (be76c77) into main (a2b1d1c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2411   +/-   ##
=======================================
  Coverage   94.76%   94.76%           
=======================================
  Files         329      329           
  Lines       48879    48880    +1     
=======================================
+ Hits        46319    46320    +1     
  Misses       2560     2560           
Flag Coverage Δ
behaviourtests 4.42% <0.00%> (-0.01%) ⬇️
unittests 95.39% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
satpy/writers/cf_writer.py 95.64% <100.00%> (+0.01%) ⬆️

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

@coveralls
Copy link

Coverage Status

Coverage: 95.349% (+0.0001%) from 95.349% when pulling be76c77 on ghiggi:fix-crs-swath-warning into a2b1d1c on pytroll:main.

@djhoese djhoese merged commit bcb53a0 into pytroll:main Mar 14, 2023
@mraspaud mraspaud added this to the v0.41.0 milestone Mar 14, 2023
@ghiggi ghiggi deleted the fix-crs-swath-warning branch March 14, 2023 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants