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

Fixes to read VII testdata #1462

Merged
merged 6 commits into from Dec 5, 2020
Merged

Conversation

ninahakansson
Copy link
Contributor

@ninahakansson ninahakansson commented Dec 1, 2020

Modifications needed to read VII testdata generated 20200921212800.

@codecov
Copy link

codecov bot commented Dec 1, 2020

Codecov Report

Merging #1462 (7d82089) into master (a18b6e2) will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1462      +/-   ##
==========================================
+ Coverage   90.52%   90.58%   +0.05%     
==========================================
  Files         238      239       +1     
  Lines       34153    34314     +161     
==========================================
+ Hits        30916    31082     +166     
+ Misses       3237     3232       -5     
Flag Coverage Δ
behaviourtests 4.53% <0.00%> (+<0.01%) ⬆️
unittests 91.06% <100.00%> (+0.05%) ⬆️

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

Impacted Files Coverage Δ
satpy/readers/vii_base_nc.py 94.17% <100.00%> (ø)
satpy/readers/vii_l1b_nc.py 96.15% <100.00%> (+0.07%) ⬆️
satpy/tests/reader_tests/test_vii_l1b_nc.py 100.00% <100.00%> (ø)
satpy/tests/test_file_handlers.py 100.00% <0.00%> (ø)
satpy/tests/reader_tests/test_abi_l1b.py 100.00% <0.00%> (ø)
satpy/tests/reader_tests/test_olci_nc.py 100.00% <0.00%> (ø)
satpy/_compat.py 100.00% <0.00%> (ø)
satpy/tests/test_readers.py 99.19% <0.00%> (+0.14%) ⬆️
satpy/readers/__init__.py 97.45% <0.00%> (+0.35%) ⬆️
satpy/readers/file_handlers.py 97.18% <0.00%> (+1.23%) ⬆️
... and 2 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 a18b6e2...7d82089. Read the comment docs.

@ninahakansson ninahakansson marked this pull request as ready for review December 2, 2020 09:31
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.

This looks good to me, just have a minor inline comment.

Just for my understanding, is this supposed to be backward compatible with older versions of the test dataset?

@@ -84,7 +84,7 @@ def get_dataset(self, dataset_id, dataset_info):
variable = self._perform_interpolation(variable)

# Perform the calibration if required
if dataset_info['calibration'] is not None:
if dataset_info.get('calibration', None) is not None:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if dataset_info.get('calibration', None) is not None:
if dataset_info.get('calibration') is not None:

None is already the default return value for get.
Also, should we issue a warning here if no calibration info is found?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Already in the vii_l1b_nc.yaml we expect some datasets to not have calibration for example "observation_azimuth".

@ninahakansson
Copy link
Contributor Author

@mraspaud I did not try to make it compatible with old versions of testdata. I think the actual data is still the same and only the format is improved.

@mraspaud
Copy link
Member

mraspaud commented Dec 2, 2020

OK, in this case should we also discard the old file pattern?

@ninahakansson
Copy link
Contributor Author

I do not know, it might be an older version but could also be the correct pattern but someone else making the files.

Copy link
Collaborator

@sjoro sjoro left a comment

Choose a reason for hiding this comment

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

LGTM

@ninahakansson
Copy link
Contributor Author

@mraspaud @sjoro I forgot to fix the brightness temperatures. Added that now.

@mraspaud mraspaud merged commit 737617c into pytroll:master Dec 5, 2020
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.

VIIl1b reader fails for testdata
3 participants