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

CI: Raise required oldest sicpy to 1.9 #6192

Merged
merged 2 commits into from
Nov 19, 2022

Conversation

janezd
Copy link
Contributor

@janezd janezd commented Nov 5, 2022

Issue

Tests on oldest fail after #6174, "Feature Statistics: Use deferred commit + minor warning fix due to the "minor warning fix" part. In scipy 1.9, scipy.stats.mode introduced argument keepdims and warns if it's not set. In Scipy 1.8 this argument did not exist.

We overlooked this because we got so used to some test configurations failing that we don't even check. Me very included.

Description of changes

We could

  • ignore this failing configuration (obviously: no),
  • remove keepdims argument and live with warning, but this would hunt us some day,
  • do something fancy to pass the argument only when necessary,
  • raise required version of Scipy from 1.3 to 1.9, which would then cascade to some other libraries (numpy 1.17 -> 1.19.5)

I see no reason against the latter. Does anybody?

@codecov
Copy link

codecov bot commented Nov 5, 2022

Codecov Report

Merging #6192 (38bea64) into master (f1726c2) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6192   +/-   ##
=======================================
  Coverage   86.66%   86.66%           
=======================================
  Files         316      316           
  Lines       68021    68021           
=======================================
  Hits        58949    58949           
  Misses       9072     9072           

@PrimozGodec
Copy link
Contributor

I do not see any problem with using a newer version of Scipy. Also, a Numpy change would not hurt; 1.19 is still quite old. Other options would bring just some complications in our code.

If we keep this change, just do not forget to make changes in requirements.txt

@PrimozGodec
Copy link
Contributor

Now we already have a known problem with reathedocs: readthedocs/readthedocs.org#9623
They, by default, still use python 3.7 to build the documentation, and we cannot directly use reathdocs.yaml file in our repository since we build multiple subprojects.

I wanted to deal with this but didn't have time yet.

@PrimozGodec
Copy link
Contributor

PrimozGodec commented Nov 11, 2022

This PR will fix the documentation at the RTD: #6197

@markotoplak markotoplak merged commit 0d3e3ee into biolab:master Nov 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants