Skip to content

Conversation

@rtavenar
Copy link
Contributor

@rtavenar rtavenar commented May 1, 2020

I changed the scipy version requirements since version scipy 1.2.1 made my POT crash (cannot remember on which call, sorry, it happened while building the docs) and the issue was fixed when upgrading to scipy 1.3

Apart from that, the main goal of this PR is to homogenize a bit the presentation in the docs.

@agramfort
Copy link
Collaborator

updating the dep should impact the test suite. Ideally we need a build running minimal dependencies

@rtavenar
Copy link
Contributor Author

rtavenar commented May 1, 2020

Did you mean something like that? And isn't it a problem that we do not test newer versions? It's unclear to me what the best option is.

And also, tests seem not to run for this PR, or maybe Github Actions is down at the moment?
EDIT: in fact they run, but the results are not reported.

It seems some of the versions specified as dependencies do not fit well together... :/

@agramfort
Copy link
Collaborator

yes something like that.

I would however not run all CIs with minimal deps. I would have only one just to check if backward compat is not broken.

You could then test if the bug your saw on your machine in repoducible on the CI and if not it's not mandatory to bump the version number

my 2c

@rtavenar
Copy link
Contributor Author

rtavenar commented May 2, 2020

OK, Will do

@rtavenar
Copy link
Contributor Author

rtavenar commented May 3, 2020

Good catch @agramfort : it must have been an issue with my local install, since the tests pass on a minimal config with scipy 1.2. I will revert the changes in the README.md and keep the test on minimal config (for numpy, scipy, cython and matplotlib), if it's OK for you.

@codecov-io
Copy link

codecov-io commented May 3, 2020

Codecov Report

Merging #163 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #163   +/-   ##
=======================================
  Coverage   92.21%   92.21%           
=======================================
  Files          15       15           
  Lines        3007     3007           
=======================================
  Hits         2773     2773           
  Misses        234      234           

@rtavenar
Copy link
Contributor Author

rtavenar commented May 3, 2020

OK, so minimal deps lead to failing tests.
It seems that with the minimal deps installed, the autograd version is old and its builtins module does not exist.

I will check that asap (yet, not now).

EDIT: it seems to be a matter of requirements from other dependencies in fact:

ERROR: autograd 1.3 has requirement numpy>=1.12, but you'll have numpy 1.11.3 which is incompatible.
ERROR: pymanopt 0.2.5 has requirement numpy>=1.16, but you'll have numpy 1.11.3 which is incompatible.

What's the best solution then? Make POT depend on numpy 1.16 or let people install older versions of autograd and pymanopt?

@rtavenar rtavenar changed the title [MRG] Improved docs and changed scipy version [WIP] Improved docs and changed scipy version May 3, 2020
@rflamary
Copy link
Collaborator

rflamary commented May 4, 2020

Hell @rtavenar and thank you for the PR.

numpy 1.16 is ok with me

I like the idea of having a one test for minimal deps. This will allow us to follow what breaks especially when new features are proposed and then we can increment those minimal deps.
But then why don't we need to put those in the setup.py then? setup.py is the one used with pip and forcing minimal version would ensure that after install the proper deps are available no?

But now what is killing me is github action and why aren't they reported in the PR! This is a major pain because they run but then i have to go the the fork to have look.

@rflamary
Copy link
Collaborator

rflamary commented May 4, 2020

Also with minimal dependencies maybe we should not install autograd or pymanop.

the tests are designed (or should be) not to fail but to skip in this configuration.

wdyt @agramfort and @rtavenar

@rflamary
Copy link
Collaborator

rflamary commented May 4, 2020

yes!

@rflamary
Copy link
Collaborator

rflamary commented May 4, 2020

OK @rtavenar,

your new test does not appear yet (until it's on the master nrach i guess) but it failed it seems. This is great because we did not have any visibility of older numpy/scipy before. Here it seems that it fails partly because np.quantile did not exist before numpy 1.15.0 so yes definitely move to 1.16 as minimal dependency.

@agramfort
Copy link
Collaborator

@rtavenar tell me when to look. I agree with the approach so far.

@rtavenar
Copy link
Contributor Author

rtavenar commented May 4, 2020

Yep @agramfort it's not ready yet, will let you know :)

@rflamary
Copy link
Collaborator

rflamary commented May 4, 2020

Looks good to me, @agramfort do you agree?

@agramfort agramfort merged commit e65c1f7 into PythonOT:master May 4, 2020
@agramfort
Copy link
Collaborator

thx a lot @rtavenar

@rflamary rflamary changed the title [WIP] Improved docs and changed scipy version [MRG] Improved docs and changed scipy version May 4, 2020
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.

4 participants