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/supress warnings in reader tests #2689

Merged
merged 36 commits into from
Dec 18, 2023
Merged

Conversation

pnuu
Copy link
Member

@pnuu pnuu commented Dec 14, 2023

This PR fixes and/or suppresses a bunch of warnings emitted by the Satpy reader tests.

  • Tests adjusted

@pnuu pnuu added component:readers component:tests cleanup Code cleanup but otherwise no change in functionality labels Dec 14, 2023
@pnuu pnuu self-assigned this Dec 14, 2023
Copy link

codecov bot commented Dec 14, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (c5c18cf) 95.32% compared to head (850b46e) 95.34%.
Report is 14 commits behind head on main.

Files Patch % Lines
satpy/readers/goes_imager_nc.py 71.42% 2 Missing ⚠️
satpy/readers/mirs.py 50.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2689      +/-   ##
==========================================
+ Coverage   95.32%   95.34%   +0.02%     
==========================================
  Files         371      371              
  Lines       52441    52379      -62     
==========================================
- Hits        49988    49943      -45     
+ Misses       2453     2436      -17     
Flag Coverage Δ
behaviourtests 4.16% <0.00%> (-0.01%) ⬇️
unittests 95.96% <97.33%> (+0.02%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@coveralls
Copy link

coveralls commented Dec 14, 2023

Pull Request Test Coverage Report for Build 7212382784

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 146 of 150 (97.33%) changed or added relevant lines in 34 files are covered.
  • 50 unchanged lines in 4 files lost coverage.
  • Overall coverage increased (+0.03%) to 95.914%

Changes Missing Coverage Covered Lines Changed/Added Lines %
satpy/readers/goes_imager_nc.py 5 7 71.43%
satpy/readers/mirs.py 2 4 50.0%
Files with Coverage Reduction New Missed Lines %
satpy/readers/goes_imager_nc.py 1 70.36%
satpy/tests/test_resample.py 2 99.57%
satpy/composites/init.py 4 93.07%
satpy/resample.py 43 88.74%
Totals Coverage Status
Change from base Build 7188190011: 0.03%
Covered Lines: 50069
Relevant Lines: 52202

💛 - Coveralls

@pnuu
Copy link
Member Author

pnuu commented Dec 14, 2023

Before this PR there were 472 warnings triggered by the reader tests, now there are 68:

  • Numpy invalid value encountered in cast/log/arcsin
  • Numpy division by zero
  • Numpy mean of empty slice
  • unregistered pytest.mark in test_mviri_l1v_fiduceo_nc
  • and some others

I'll remove the one FIXME and create a fix in Pyresample.

@pnuu pnuu marked this pull request as ready for review December 14, 2023 12:12
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.

Great work, just one comment about alleviating some of the pyproj warnings. Otherwise LGTM.

Comment on lines 143 to 147
with warnings.catch_warnings():
warnings.filterwarnings("ignore",
message=r"You will likely lose important projection information",
category=UserWarning)
proj_dict = area_def.proj_dict
Copy link
Member

Choose a reason for hiding this comment

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

I think the warning would disappear if the crs was passed instead of the proj dict, as proj4_radius_parameters accepts also CRSs: https://github.com/pytroll/pyresample/blob/main/pyresample/utils/proj4.py#L76-L91

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I'll have a look.

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems I could get all the tested things from area_def.crs.to_cf() apart from the units. There is LENGTHUNIT["metre",...] in crs_wkt, but checking for that seems like a cludge. So it's either using proj_dict or doing "metre" in area_def.crs.to_cf()['crs_wkt'] 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Maybe crs.axis_info[0].unit_name?

Copy link
Member

Choose a reason for hiding this comment

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

But how about creating an expected crs instead, and comparing it to area_def.crs?
eg:

expected_crs = CRS.from_dict(dict(a=6378137.0, b=6356752.3, h=35785863.0, lon_0=140.7, proj="geos", units="m"))
assert area_def.crs == expected_crs

Copy link
Member Author

Choose a reason for hiding this comment

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

Too simple 🙈 Adjusted in 24422ad

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I didn't comment on the other ones, but there are a bunch other places where this could be used instead of suppressing the warning...

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to go the "do not suppress" route, but evidently I missed this way. I'll start combing through the changes for these.

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated the proj_dict usages to CRS as much as possible. The remaining are used either because the original projection needs to be modified (in nwcsaf_nc) and the dict is just too simple not to be used, or because the CRS objects differ even if the proj dicts are identical (oceancolorcci_l3_nc).

@mraspaud mraspaud merged commit 4c30838 into pytroll:main Dec 18, 2023
17 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Code cleanup but otherwise no change in functionality component:readers component:tests
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants