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

MAINT: linalg: get rid of calc_lwork.f #3871

Merged
merged 3 commits into from
Aug 14, 2014

Conversation

pv
Copy link
Member

@pv pv commented Aug 13, 2014

Call LAPACK routines with lwork=-1 to get recommended work array sizes,
instead of using calc_lwork.f (which corresponds to some old LAPACK
version and may not be optimal for newer ones).

Also remove the LWORK check from zgesdd wrapper --- LAPACK makes the
necessary checks itself.

(This is more of a cleanup rather than a bug fix.)

Call LAPACK routines with lwork=-1 to get recommended work array sizes,
instead of using calc_lwork.f (which corresponds to some old LAPACK
version and may not be optimal for newer ones).

Also remove the LWORK check from zgesdd wrapper --- LAPACK makes the
necessary checks itself.
@pv pv added the PR label Aug 13, 2014
@argriffing
Copy link
Contributor

This is off-topic but are PR labels still needed with github's updated interface?

@pv
Copy link
Member Author

pv commented Aug 13, 2014

The PR tag is not needed now that the interface is a bit more sane.

@pv pv removed the PR label Aug 13, 2014
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.0%) when pulling ebc5ffe on pv:eliminate-calc-lwork into 47137df on scipy:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.0%) when pulling ebc5ffe on pv:eliminate-calc-lwork into 47137df on scipy:master.

argriffing added a commit that referenced this pull request Aug 14, 2014
MAINT: linalg: get rid of calc_lwork.f
@argriffing argriffing merged commit 0062373 into scipy:master Aug 14, 2014
@argriffing
Copy link
Contributor

Good riddance to calc_lwork! I would feel a little better if TravisCI knew about more blas/lapack implementations, but it passes the tests so I've merged it.

@WarrenWeckesser
Copy link
Member

scipy.linalg.calc_lwork (an implicitly public part of the API) was being used in the wild: statsmodels/statsmodels#2184

@argriffing has already fixed statsmodels for its next release (statsmodels/statsmodels#2160), but the missing function will still cause a problem for anyone trying to use scipy 0.15 with a current or older version of statsmodels. Should we restore a version of calc_lwork (with a deprecation warning) in 0.15.1?

@WarrenWeckesser
Copy link
Member

Ripple effects: bokeh/bokeh#1695

@matthew-brett
Copy link
Contributor

It would be very good to have some early warning of things like this.

Maybe some extension of the testing here:

There's some discussion about this going on at statsmodels/statsmodels#2184

@pv
Copy link
Member Author

pv commented Jan 15, 2015

Possibly --- also pyamg and krypy appear to use it. Can you open a
ticket? Probably best to also check if there's something other what we
messed.
.
However, this is a bit tricky, because the reason for removing
calc_lwork was that it did not work correctly (not in sync with
current LAPACK) and restoring it will then not ensure the old code works
correctly.
.
We'd need to rewrite the "new deprecated" calc_lwork module using the
new *_lwork functions added in the f2py wrappers.

@pv
Copy link
Member Author

pv commented Jan 15, 2015

Yes, I think we want an automated whole-stack test suite that can be easily run at least before releases. The test suite doesn't need to have the latest versions of the downstream libraries (in fact it could test several old versions too).

@matthew-brett
Copy link
Contributor

The scipy-stack system does this already, but you need to do more or less manually. How about adding that to the RC process?

@pv
Copy link
Member Author

pv commented Jan 15, 2015

Sure. Are there instructions somewhere how it works?

@matthew-brett
Copy link
Contributor

I'll add those on the README, and invite you as Admin to the repo. Ralf is already an Admin.

@matthew-brett
Copy link
Contributor

Filled out README.rst at https://github.com/MacPython/scipy-stack-osx-testing ; does that help?

@pv
Copy link
Member Author

pv commented Jan 15, 2015

Ok, thanks. Quite a nice bit of infra you have there :)

@argriffing
Copy link
Contributor

Would it make sense to somehow temporarily roll back the scipy version on pypi so it's not breaking 8,000 people's work per day?

@matthew-brett
Copy link
Contributor

Surely a very fast point release would be a better option?

@pv
Copy link
Member Author

pv commented Jan 15, 2015

To avoid this crap in the future, we could just break everything at
once, i.e., in Scipy 0.16.0 prefix all non-public modules with '_' and
add compatilibity stub modules that emit noisy deprecation/futurewarnings.

@argriffing
Copy link
Contributor

Surely a very fast point release would be a better option?

In that case would it make sense to release a very fast band-aid point release, rather than a very fast point release that attempts to make the correct fix that @pv describes as a bit tricky? Maybe just add calc_lwork = None or a more ambitious stub module into scipy.linalg so that people who install both scipy and statsmodels directly or indirectly from pypi on travis won't hit an import error? @josef-pkt could correct me, but I think the only actual usage (as opposed to import) of calc_lwork within statsmodels is in some 'dead' code.

@pv
Copy link
Member Author

pv commented Jan 15, 2015

I think calc_lwork is used in real code in the other projects, however.
.
If we want minimal work, we can just restore calc_lwork in all its buggy
glory --- the bugs iirc affect only a few of the routines, and the
correct fix is to start using the new lwork-computation routines.

@matthew-brett
Copy link
Contributor

How about applying that patch on top of 0.15.0 and releasing today as 0.15.1?

@josef-pkt
Copy link
Member

calc_lwork in statsmodels is in an old unused function that I had forgotten about, but unfortunately the module is always imported, and breaks any standard use of statsmodels (api.py)
Unless there is a scipy release to fix this, we will have to make a statsmodels point release. The next release of statsmodels is scheduled to be in about 1.5 months.

@pv
Copy link
Member Author

pv commented Jan 15, 2015

I'd suggest to (i) 0.15.1 with the old+buggy calc_lwork, and (ii) get it out during the weekend and check see if something else is also on fire (I'm not going to get it done today even as a source-only release).

@matthew-brett
Copy link
Contributor

We can always release 0.15.2 if anything else comes up. I can do the OSX binary builds of course, is there anything else I can help with?

@rgommers
Copy link
Member

@pv sounds good. The other thing that needs fixing it looks like is file permissions: gh-4375.

@rgommers
Copy link
Member

Or maybe you fixed that already for 0.14.1 and 0.15.0? Then that issue can be closed.

@rgommers
Copy link
Member

+1 for underscoring everything at once and adding warnings for the regular imports.

@matthew-brett
Copy link
Contributor

I added statsmodels to the scipy-stack-osx-testing test set. At the moment tests are failing because of this issue:

https://travis-ci.org/MacPython/scipy-stack-osx-testing/builds/47383316

@rgommers
Copy link
Member

@matthew-brett thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Items related to regular maintenance tasks scipy.linalg
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants