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

Cleanup various warnings encountered during tests #2625

Merged
merged 17 commits into from
Nov 9, 2023

Conversation

djhoese
Copy link
Member

@djhoese djhoese commented Nov 1, 2023

Fixes various warnings in regular CI and some that have turned into errors in unstable CI.

Let's see how many I can get in a single go...

  • Closes #xxxx
  • Tests added
  • Fully documented
  • Add your name to AUTHORS.md if not there already

@djhoese djhoese added bug cleanup Code cleanup but otherwise no change in functionality labels Nov 1, 2023
@djhoese djhoese self-assigned this Nov 1, 2023
Copy link

codecov bot commented Nov 1, 2023

Codecov Report

Merging #2625 (0ef3e6b) into main (ee63577) will increase coverage by 0.02%.
The diff coverage is 97.33%.

@@            Coverage Diff             @@
##             main    #2625      +/-   ##
==========================================
+ Coverage   95.18%   95.20%   +0.02%     
==========================================
  Files         354      354              
  Lines       51316    51368      +52     
==========================================
+ Hits        48846    48906      +60     
+ Misses       2470     2462       -8     
Flag Coverage Δ
behaviourtests 4.24% <2.66%> (-0.01%) ⬇️
unittests 95.83% <97.33%> (+0.02%) ⬆️

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

Files Coverage Δ
satpy/readers/abi_l1b.py 98.87% <100.00%> (ø)
satpy/readers/fci_l1c_nc.py 97.61% <100.00%> (-0.01%) ⬇️
satpy/readers/goes_imager_hrit.py 95.67% <100.00%> (+0.44%) ⬆️
satpy/readers/hdfeos_base.py 92.66% <100.00%> (+0.02%) ⬆️
satpy/readers/mersi_l1b.py 97.02% <100.00%> (ø)
satpy/readers/utils.py 92.95% <100.00%> (ø)
satpy/readers/viirs_atms_sdr_base.py 94.52% <100.00%> (+0.02%) ⬆️
satpy/tests/modifier_tests/test_parallax.py 99.71% <100.00%> (+<0.01%) ⬆️
satpy/tests/reader_tests/_li_test_utils.py 99.23% <ø> (ø)
...y/tests/reader_tests/modis_tests/test_modis_l1b.py 99.18% <100.00%> (+0.02%) ⬆️
... and 6 more

... and 3 files with indirect coverage changes

@coveralls
Copy link

coveralls commented Nov 1, 2023

Pull Request Test Coverage Report for Build 6816253255

  • 73 of 75 (97.33%) changed or added relevant lines in 15 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.02%) to 95.782%

Changes Missing Coverage Covered Lines Changed/Added Lines %
satpy/tests/utils.py 19 21 90.48%
Files with Coverage Reduction New Missed Lines %
satpy/writers/awips_tiled.py 1 84.01%
Totals Coverage Status
Change from base Build 6737851658: 0.02%
Covered Lines: 49032
Relevant Lines: 51191

💛 - Coveralls

@djhoese djhoese marked this pull request as ready for review November 2, 2023 01:39
@djhoese
Copy link
Member Author

djhoese commented Nov 2, 2023

This doesn't fix all, but fixes a large chunk of the warnings in the stable environments and the errors in the unstable. There is a patch that isn't released for h5py that will fix the errors about byte order. The skyfield package is not numpy 2 compatible yet and I'm not sure if/what the timeline is there, but even fixing it locally for the error we're seeing I'm seeing strange errors afterward about pandas/xarray trying to interpret our crs coordinate as a datetime and completely crashing in the attempt (it is meant to just "try" the conversion). I haven't looked at too many others.

Most of the ones I fixed seemed to be related to numpy warnings being filtered by dask or xarray and hiding the new numpy dtype casting errors that numpy 2.0 is throwing at us. We apparently do a lot of np.uint(-1) type stuff either on purpose or by accident.

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, just one question

satpy/writers/awips_tiled.py Show resolved Hide resolved
@djhoese djhoese marked this pull request as draft November 2, 2023 13:43
@djhoese djhoese marked this pull request as ready for review November 2, 2023 17:41
@djhoese
Copy link
Member Author

djhoese commented Nov 2, 2023

Ok I think this is as close as we're going to get to numpy 2.0 compatibility for now. We're waiting on:

  1. New version of h5py that includes a fix for the newbyteorder stuff.
  2. Fixes for skyfield to not import float_ from numpy.

Note: With numpy 2.0 we are going to very likely be upcasting a lot of arrays by accident. Much of this is due to an xarray "bug". See pydata/xarray#8402 for more details.

Copy link
Member

@sfinkens sfinkens left a comment

Choose a reason for hiding this comment

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

Thanks a lot for tackling all those issues!

@djhoese
Copy link
Member Author

djhoese commented Nov 3, 2023

Based on some comments by @gerritholl on slack, I added some xfails for the specific cases where we expect the tests to fail. Those being:

  1. The unstable environment, with numpy 2.x, with skyfield failing to import
  2. The unstable environment, with numpy 2.x, and an old version of h5py (waiting on a release with a fix)

Let's see how CI goes...

@djhoese
Copy link
Member Author

djhoese commented Nov 4, 2023

Ok I'm pretty sure this PR is ready now. As mentioned above, the known numpy 2 related failures are marked as xfail for only the UNSTABLE environment and only with specific versions or specific known incompatibilities. Once those are fixed or new releases come out then the tests should be expected to pass.

@djhoese djhoese requested a review from mraspaud November 8, 2023 16:38
@djhoese
Copy link
Member Author

djhoese commented Nov 8, 2023

@mraspaud could you re-review this since I've done a lot more work since your original approval.

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 a lot for the hard work! I just have two minor things inline, but no show-stopper

@djhoese
Copy link
Member Author

djhoese commented Nov 9, 2023

Ok, with your two comment threads being resolved and your review being approval I think I'll merge this today. It's a little late for European comment, but if anyone else wants to provide feedback let me know what you think.

@djhoese
Copy link
Member Author

djhoese commented Nov 9, 2023

I did one more commit to make codebeat happier.

Edit: Haha codebeat now says that goes_imager_hrit.py has been removed. That's one way to resolve code style issues...but this is also not true so 🤷‍♂️

@djhoese djhoese merged commit 1e0cf20 into pytroll:main Nov 9, 2023
19 checks passed
@djhoese djhoese deleted the bugfix-np2-compat branch November 9, 2023 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cleanup Code cleanup but otherwise no change in functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants