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 assumption that arrays have 2+ dimensions in CF writer #2451

Merged
merged 6 commits into from
Apr 20, 2023

Conversation

pnuu
Copy link
Member

@pnuu pnuu commented Apr 19, 2023

The CF writer currently has an assumption that all arrays have both x and y coordinates. This might not be intentional, as the only place this is enforced is in a private method where coordinate units are check when trying to determine whether the data are projected or not.

This had come up in https://github.com/foua-pps/level1c4pps/ which uses Satpy to convert data to NWC SAF PPS compatible format.

The test I added is very simple, and without the try/except I added in this PR it'd fail with KeyError: 'x'.

Ping also to @ninahakansson

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, just one styling comment.

@pnuu pnuu requested a review from sfinkens April 19, 2023 12:56
@mraspaud
Copy link
Member

3 tests failing now...

@mraspaud mraspaud added this to the v0.42.0 milestone Apr 20, 2023
@pnuu
Copy link
Member Author

pnuu commented Apr 20, 2023

The failing tests were because of the netCDF compression settings not working with the latest XArray version.

@codecov
Copy link

codecov bot commented Apr 20, 2023

Codecov Report

Merging #2451 (6dae14e) into main (f7c8931) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2451      +/-   ##
==========================================
- Coverage   94.81%   94.81%   -0.01%     
==========================================
  Files         339      339              
  Lines       49391    49343      -48     
==========================================
- Hits        46832    46784      -48     
  Misses       2559     2559              
Flag Coverage Δ
behaviourtests 4.42% <0.00%> (+<0.01%) ⬆️
unittests 95.43% <100.00%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
satpy/tests/writer_tests/test_cf.py 99.57% <100.00%> (-0.03%) ⬇️
satpy/writers/cf_writer.py 95.66% <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.

@pnuu
Copy link
Member Author

pnuu commented Apr 20, 2023

I arranged the imports in test_cf.py so CodeScene complains about one thing less...

@coveralls
Copy link

Coverage Status

Coverage: 95.383% (-0.005%) from 95.388% when pulling 6dae14e on pnuu:bugfix-cf-writer-1D-array into f7c8931 on pytroll:main.

@mraspaud mraspaud merged commit 5310f1a into pytroll:main Apr 20, 2023
@pnuu pnuu deleted the bugfix-cf-writer-1D-array branch April 20, 2023 07:16
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.

3 participants