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

Remove unnecessary string decode in agri_l1 reader #1459

Merged
merged 1 commit into from
Jan 5, 2021

Conversation

jactry
Copy link
Contributor

@jactry jactry commented Nov 29, 2020

This fixs a regression introduced by commit 14d9681.

  • Passes flake8 satpy
  • Add your name to AUTHORS.md if not there already

@codecov
Copy link

codecov bot commented Nov 29, 2020

Codecov Report

Merging #1459 (4327226) into master (a18b6e2) will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1459      +/-   ##
==========================================
- Coverage   90.52%   90.52%   -0.01%     
==========================================
  Files         238      238              
  Lines       34153    34149       -4     
==========================================
- Hits        30916    30912       -4     
  Misses       3237     3237              
Flag Coverage Δ
behaviourtests 4.53% <ø> (+<0.01%) ⬆️
unittests 91.00% <ø> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
satpy/readers/agri_l1.py 99.15% <ø> (-0.03%) ⬇️

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 a18b6e2...4327226. Read the comment docs.

@djhoese djhoese requested a review from zxdawn November 29, 2020 15:42
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.

I believe @zxdawn recently ran into this issue as well. The main issue is newer versions of h5py loading/decoding strings differently. After investigating a bit with @zxdawn on slack, we weren't really sure why these strings were encoded with gbk anyway since they are all ASCII characters.

I'm OK with this PR, but am still curious why these strings ever needed to be decoded specifically with gbk in the first place.

@zxdawn
Copy link
Member

zxdawn commented Dec 2, 2020

@djhoese I can test different versions of h5py and report here later.

@mraspaud
Copy link
Member

@zxdawn any progress here?

@zxdawn
Copy link
Member

zxdawn commented Dec 24, 2020

@mraspaud It's not related to the version of h5py.

I have tested satpy==0.23.0, h5py==2.10.0 and satpy==0.23.0, h5py==3.1.0, they both works well without decoding:

'band_names': 'band12(band number is range from 1 to 14)'
'long_name': '10.8um channel 4km image data layer'

Then, if I test satpy==0.21.0, h5py==2.10.0, these attrs are bytes without decoding:

'band_names': b'band12(band number is range from 1 to 14)'
'long_name': b'10.8um channel 4km image data layer'

Anyway, LGTM for the newest satpy.

@djhoese
Copy link
Member

djhoese commented Jan 5, 2021

Thanks. Looks good.

@djhoese djhoese changed the title agri_l1: Don't try to decode string. Remove unnecessary string decode in agri_l1 reader Jan 5, 2021
@djhoese djhoese merged commit d2e7d46 into pytroll:master Jan 5, 2021
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