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

Only add longitude/latitude variables in cf_writer if they are not included already. #1357

Merged
merged 5 commits into from Sep 16, 2020

Conversation

ninahakansson
Copy link
Contributor

@ninahakansson ninahakansson commented Sep 10, 2020

Only add lon/lat variables if they are not included already.
Level1c4pps ended up with both lat/lon and latitude/longitude coordinates.

  • Tests added
  • Passes flake8 satpy

Level1c4pps ended up with both lat/lon and latitude/longitude coordinates.
@coveralls
Copy link

coveralls commented Sep 10, 2020

Coverage Status

Coverage increased (+0.02%) to 90.484% when pulling 344d78f on ninahakansson:fix_cf_writer_latlons into 5cf76e6 on pytroll:master.

@codecov
Copy link

codecov bot commented Sep 10, 2020

Codecov Report

Merging #1357 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1357      +/-   ##
==========================================
+ Coverage   90.46%   90.48%   +0.01%     
==========================================
  Files         228      228              
  Lines       33211    33269      +58     
==========================================
+ Hits        30045    30103      +58     
  Misses       3166     3166              
Impacted Files Coverage Δ
satpy/tests/writer_tests/test_cf.py 99.64% <100.00%> (+0.02%) ⬆️
satpy/writers/cf_writer.py 94.09% <100.00%> (+0.20%) ⬆️
satpy/tests/reader_tests/test_utils.py 100.00% <0.00%> (ø)
satpy/readers/ahi_gridded.py
satpy/tests/reader_tests/test_ahi_gridded.py
...tpy/tests/reader_tests/test_ahi_l1b_gridded_bin.py 99.34% <0.00%> (ø)
satpy/readers/ahi_l1b_gridded_bin.py 99.05% <0.00%> (ø)
satpy/readers/utils.py 90.17% <0.00%> (+0.17%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5cf76e6...344d78f. Read the comment docs.

'standard_name': "latitude",
'units': 'degrees_north'},
name='latitude')
if ('lat' in dataarray and 'lon' in dataarray) or ('latitude' in dataarray and 'longitude' in dataarray):
Copy link
Member

Choose a reason for hiding this comment

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

To make this clearer, could we have if ('lat' in dataarray.coords and...?

@mraspaud
Copy link
Member

@ninahakansson Thanks for adding this. Do you know where the lon and lat variants are coming from? afaik we use the full longitude and latitude names in satpy.

@ninahakansson
Copy link
Contributor Author

The lat/lon is from level1c4pps and from PPS. So in one way they do not belong here I guess. We have the include_lonlats attribute that in level1c4pps is set to False to avoid adding lat/lons again. But in area2cf this attribute is ignored if the data is in Swath definition. This line:
if isinstance(dataarray.attrs['area'], SwathDefinition) or strict:
Changing the or => and would also solve my problem. But I am afraid that would break something for other users.

@mraspaud
Copy link
Member

@ninahakansson changing or with and would change the meaning of this. strict means that we want to include lons/lats even if there is a valid grid mapping.

@ninahakansson ninahakansson marked this pull request as ready for review September 14, 2020 13:19
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.

Good start. I'm missing a test that describe your use case, with the renaming of the coordinates. Maybe that would be testing the _collect_datasets function?

satpy/tests/writer_tests/test_cf.py Outdated Show resolved Hide resolved
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, thanks for fixing the lon/lat overwrite!

@mraspaud
Copy link
Member

@sfinkens do you want to check this against pygac-fdr before I merge this?

@sfinkens
Copy link
Member

No, go ahead! I'll see what the system tests will find.

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