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

Delete unnecessary coordinates in tropomi reader #1589

Merged
merged 1 commit into from
Mar 10, 2021

Conversation

zxdawn
Copy link
Member

@zxdawn zxdawn commented Mar 9, 2021

The coordinates of longitude and latitude shouldn't be linked to themselves again.

And this PR can fix this error of scn.save_datasets() when loading longitude or latitude:

  File "test_save.py", line 77, in <module>
    scn.save_datasets(filename='./output/test.nc')
  File "/public/home/zhangxin/new/miniconda3/envs/s5p_tobac/lib/python3.8/site-packages/satpy/scene.py", line 1065, in save_datasets
    return writer.save_datasets(dataarrays, compute=compute, **save_kwargs)
  File "/public/home/zhangxin/new/miniconda3/envs/s5p_tobac/lib/python3.8/site-packages/satpy/writers/cf_writer.py", line 721, in save_datasets
    res = dataset.to_netcdf(filename, engine=engine, group=group_name, mode='a', encoding=encoding,
  File "/public/home/zhangxin/new/miniconda3/envs/s5p_tobac/lib/python3.8/site-packages/xarray/core/dataset.py", line 1689, in to_netcdf
    return to_netcdf(
  File "/public/home/zhangxin/new/miniconda3/envs/s5p_tobac/lib/python3.8/site-packages/xarray/backends/api.py", line 1107, in to_netcdf
    dump_to_store(
  File "/public/home/zhangxin/new/miniconda3/envs/s5p_tobac/lib/python3.8/site-packages/xarray/backends/api.py", line 1142, in dump_to_store
    variables, attrs = conventions.encode_dataset_coordinates(dataset)
  File "/public/home/zhangxin/new/miniconda3/envs/s5p_tobac/lib/python3.8/site-packages/xarray/conventions.py", line 806, in encode_dataset_coordinates
    return _encode_coordinates(
  File "/public/home/zhangxin/new/miniconda3/envs/s5p_tobac/lib/python3.8/site-packages/xarray/conventions.py", line 765, in _encode_coordinates
    written_coords.update(attrs["coordinates"].split())
AttributeError: 'list' object has no attribute 'split'

@zxdawn zxdawn requested review from djhoese and mraspaud as code owners March 9, 2021 12:43
@zxdawn zxdawn self-assigned this Mar 9, 2021
@djhoese
Copy link
Member

djhoese commented Mar 9, 2021

My long term goal (a wish) has been to make the base reader support this type of logic (a coordinate variable depending on itself for coordinates). I don't expect you to fix it and I don't expect myself to have the time to do it right now either so I am OK with this PR if it fixes all of this CF writer stuff.

That said, I'm still not sure I understand why the CF writer chokes on this.

@codecov
Copy link

codecov bot commented Mar 9, 2021

Codecov Report

Merging #1589 (2f5aeef) into master (d7fbbdf) will increase coverage by 1.22%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1589      +/-   ##
==========================================
+ Coverage   91.62%   92.84%   +1.22%     
==========================================
  Files         246      251       +5     
  Lines       36034    37193    +1159     
==========================================
+ Hits        33016    34532    +1516     
+ Misses       3018     2661     -357     
Flag Coverage Δ
behaviourtests 4.47% <ø> (+0.05%) ⬆️
unittests 92.98% <ø> (+0.89%) ⬆️

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

Impacted Files Coverage Δ
satpy/readers/file_handlers.py 95.77% <0.00%> (-1.41%) ⬇️
satpy/readers/satpy_cf_nc.py 97.56% <0.00%> (-1.02%) ⬇️
satpy/tests/test_yaml_reader.py 99.53% <0.00%> (-0.31%) ⬇️
satpy/tests/writer_tests/test_awips_tiled.py 98.23% <0.00%> (-0.26%) ⬇️
satpy/resample.py 89.50% <0.00%> (-0.20%) ⬇️
satpy/readers/nucaps.py 89.14% <0.00%> (-0.05%) ⬇️
satpy/readers/__init__.py 97.42% <0.00%> (-0.04%) ⬇️
satpy/tests/test_scene.py 99.72% <0.00%> (-0.01%) ⬇️
satpy/plugin_base.py 88.00% <0.00%> (ø)
satpy/readers/ami_l1b.py 96.99% <0.00%> (ø)
... and 72 more

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 d7fbbdf...2f5aeef. Read the comment docs.

@zxdawn
Copy link
Member Author

zxdawn commented Mar 9, 2021

@djhoese If these coordinates are not deleted, the problem is caused by https://github.com/pydata/xarray/blob/50d97e9d35bac783850827fa66ff5eb768e62905/xarray/conventions.py#L764-L765.

attrs["coordinates"] is a list ['longitude', 'latitude'] which doesn't support .split()

@djhoese
Copy link
Member

djhoese commented Mar 9, 2021

🤔 So maybe the CF writer should check for bad coordinates and remove them? But then again, the CF writer shouldn't have to fix every piece of metadata...

@zxdawn zxdawn mentioned this pull request Mar 9, 2021
1 task
@djhoese djhoese changed the title Delete useless coordinates in tropomi yaml Delete unnecessary coordinates in tropomi reader Mar 10, 2021
Copy link
Member

@djhoese djhoese left a comment

Choose a reason for hiding this comment

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

Looks good enough to me. This is a complicated topic that has a lot of parts (as far as the problem that is actually trying to be solved). This seems like the simplest fix for now until we get better coordinate handling in the base reader class.

@djhoese djhoese merged commit e796be5 into pytroll:master Mar 10, 2021
@zxdawn zxdawn deleted the tropomi_yaml branch March 11, 2021 01: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.

2 participants