nci / scores

scores: metrics for the verification, evaluation and optimisation of forecasts, predictions or models
https://scores.readthedocs.io/
Apache License 2.0
24 stars 9 forks source link

[JOSS] Fix warnings in test suite and examples #554

Open savente93 opened 2 weeks ago

savente93 commented 2 weeks ago

This issue was filed as part of my JOSS review https://github.com/openjournals/joss-reviews/issues/6889

There are a few warnings generated by both the tutorials and the test suite. They are very superficial warnings, such as deprecation warnings, so it is not a critical issue, but it is good practice to keep your examples and test suite warning free if possible. I realize this is a bit of a pet peeve of mine, so the submission acceptance is not conditional on fixing this, but I would recommend it, as it's an easy win and gives the package a much more polished impression for new comers.

tennlee commented 1 week ago

Thanks for your feedback and I also prefer to handle all warnings. In this case, I will need to do some investigation in how to do this before I can proceed. As such it might take me some time. I thought I should provide a response in the meantime so it's clear we intend to pick up the issue.

My results are that the Python 3.12 tests are showing no warnings, whereas I see warnings on the other Python versions. I don't know the best way to handle version-specific warning handling across the test suite. The warnings all relate to code in dependencies so all we could do is catch them as expected for that version.

The warnings in the tutorials bother me more than the warnings in the tests, since they are presented more directly to scientific users who may be less able to quickly identify what to disregard and what to worry about. I will probably raise a separate issue to track the notebooks so that I can chunk up the work more easily.

savente93 commented 1 week ago

My results are that the Python 3.12 tests are showing no warnings, whereas I see warnings on the other Python versions. I don't know the best way to handle version-specific warning handling across the test suite. The warnings all relate to code in dependencies so all we could do is catch them as expected for that version.

If they are warnings that you are aware of and know don't impact you there are several things you can do:

  1. you could add filter warnings for pytest in your pyproject.toml (see https://docs.pytest.org/en/7.1.x/how-to/capture-warnings.html#deprecationwarning-and-pendingdeprecationwarning)
  2. You could add filter warnings to your __init__.py (see https://docs.python.org/3/library/warnings.html#overriding-the-default-filter )
  3. Not by any means necessary, but if you're feeling up to it you could go to upstream and file either a PR or an issue to fix the warning

These are the options I usually go for, I think which you choose is mostly up to preference.

tennlee commented 1 week ago

Based on a review of readthedocs, which will be based on the latest versions of everything, the only tutorial showing a warning Pearson's correlation coefficient, which has a deprecation warning. I'll raise a new issue for that specifically and start there.

tennlee commented 1 week ago

I believe I have now addressed all of the issues in the tutorials, at least for Python 3.12. This is the version used to build the documentation and the error no longer shows in readthedocs latest. It is also the version developers/users should get in new environments.

savente93 commented 1 week ago

The tutorials indeed seem to be warning free so that is very nice. Though I'm still seeing some deprecatio warnings in the test suite:

DeprecationWarning: `trapz` is deprecated. Use `trapezoid` instead, or one of the numerical integration functions in `scipy.integrate`.

see pytest section at https://github.com/nci/scores/actions/runs/9645482074/job/26599796094

tennlee commented 1 week ago

Thanks. I will work through the test suites next and report back.