8000 MAINT: Rebuild lapack lite by eric-wieser · Pull Request #8381 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

MAINT: Rebuild lapack lite #8381

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Feb 20, 2017
Merged

MAINT: Rebuild lapack lite #8381

merged 5 commits into from
Feb 20, 2017

Conversation

eric-wieser
Copy link
Member
@eric-wieser eric-wieser commented Dec 13, 2016 •
edited
Loading

This (the first four commits) is based on top of #8377, which makes it possible to run this script at all. (rebased now that those are merged)

This will give a baseline to compare #8376 against. This does not produce the same output as what is committed, sadly. There are a number of possible reasons for this:

  • The script is non-deterministic, and depends on something like dictionary ordering
  • The original used a patched version of f2c, but that patch was not recorded, and patching f2c as part of the build process is a lot of effort just to squelch some warnings
  • The version of lapack originally transpiled is not the one it says it is

@eric-wieser
Copy link
Member Author
eric-wieser commented Dec 13, 2016 •
edited
Loading

Hmm, those tests were cancelled for some reason. git commit --amend --no-edit && git push -f...

@eric-wieser
Copy link
Member Author

@pv @ovillellas: Don't suppose you remember how you built these files in c679f7b?

Seems that somehow blas_lite.c originally did not contain a definition for xerbla_, but now it does (which causes the CI failure). Was this function manually removed from blas_lite?

@eric-wieser eric-wieser force-pushed the rebuild-lapack_lite branch 2 times, most recently from 39bcf29 to f97355f Compare December 14, 2016 01:32
@eric-wieser
Copy link
Member Author

This latest build strips out cgelsd, clals0, clalsa, clalsd, sgelsd, slals0, slalsa, and slalsd - none of which are used.

@eric-wieser
Copy link
Member Author

Seems that somehow blas_lite.c originally did not contain a definition for xerbla_, but now it does

Nope, someone had manually added an #if 0. Fixed in 0a4c136.

@eric-wieser
Copy link
Member Author

Awesome, that actually worked! Ready for review, I think.

@rgommers
Copy link
Member

The last paragraphy of README says:

A slightly-patched ``f2c`` was used to add parentheses around ``||`` expressions
and the arguments to ``<<`` to silence gcc warnings. Edit
the ``src/output.c`` in the ``f2c`` source to do this.

The diff in this PR doesn't have these additional parentheses.

@rgommers
Copy link
Member

It would also be useful to add some instructions to README on how to test lapack_lite. In our CI setup it's not tested at all I believe.

@eric-wieser
Copy link
Member Author

The diff in this PR doesn't have these additional parentheses.

Correct. Manually obtaining the f2c source, patching without instruction, and rebuilding seems like a lot of busywork to fix unimportant warnings.

@eric-wieser
Copy link
Member Author
eric-wieser commented Dec 29, 2016 •
edited
Loading

@rgommers: Are you content for me to just remove these lines from the README?

@eric-wieser
Copy link
Member Author
8000 eric-wieser commented Dec 29, 2016

It would also be useful to add some instructions to README on how to test [it]

There's nothing really to say here. You test the generated files by building the generated files, then running the numpy test suite.

@eric-wieser
MAINT: retranspile lapack from the supposed source
35ea1c9 
The source used is http://archive.debian.org/debian/pool/main/l/lapack3/lapack3_3.0.20000531a.orig.tar.gz. Originally this was done with a patched f2c, but if the patch isn't provided in the source tree, there's no sensible way to use it
@eric-wieser
BUG: Do not repeat xerbla_, which we deliberately replace
b973196 
This is also defined in python_xerbla.c, where we redefine a python-compatible version
@eric-wieser
MAINT: Sort the functions, for ease of diffing
debb7a3
@eric-wieser
BUG: add missing routines to the transpilation list
224abf8 
These are taken from lapack_litemodule.c and umath_linalg.c.src
@eric-wieser eric-wieser mentioned this pull request Jan 5, 2017
Merged
@charris
Copy link
8000 Member
charris commented Jan 21, 2017

What is the motivation for this, what does it fix/add?

@eric-wieser
Copy link
Member Author
eric-wieser commented Jan 22, 2017 •
edited
Loading

The motivation is that provides a bisection point when upgrading to a newer version of lapack (#8376), which is required to fix a problem mentioned in an issue I think I linked above.

Otherwise, it becomes hard to distinguish "this version doesn't work" from "these build tools do not work".

@eric-wieser
Copy link
Member Author

@charris: The underlying issue requiring an upgrade of lapack-lite is discussed here

@charris charris changed the title Rebuild lapack lite MAINT: Rebuild lapack lite Feb 18, 2017
@eric-wieser eric-wieser mentioned this pull request Feb 20, 2017
Closed
@charris
Copy link
Member
8000 charris commented Feb 20, 2017

@eric-wieser The way to test this is to force a build that uses lapack_lite. For instance

BLAS=None LAPACK=None ATLAS=None python setup.py build

will not use any system libraries.

@eric-wieser
Copy link
Member Author

@charris: This is already tested by appveyor, I believe. (Since it failed until I got it right)

@charris
Copy link
Member
charris commented Feb 20, 2017

You should be able to define those variables in the env in a (new) travis test.

@eric-wieser
Copy link
Member Author

But adding it explicitly to travis seems like a good idea anyway, so I'll do that too

@charris
Copy link
Member
charris commented Feb 20, 2017

Yep, better done on purpose than by accident.

@eric-wieser
Copy link
Member Author

@charris: Alright, done and passing

@charris charris merged commit 0a252f5 into numpy:master Feb 20, 2017
@charris
Copy link
Member
charris commented Feb 20, 2017

Let's get this underway. Thanks Eric.

@charris
Copy link
Member
charris commented Feb 21, 2017

About stripping out cgelsd, clals0, clalsa, clalsd, sgelsd, slals0, slalsa, and slalsd, I note that dgelsd is present and presumably used, although zgelsd is missing. Since in most cases both single and double are now supported -- originally just double -- it may be that the single versions are missing through oversight and in the new version you may want to 67E6 keep all of those.

@eric-wieser
Copy link
Member Author
eric-wieser commented Feb 21, 2017 •
edited
Loading

zgelsd is used in np.linalg, so I'd sure hope it's not missing!

But right now, lstsq only uses [dz]gelsd, and doesn't have any special casing for single/complex64

@charris
Copy link
Member
charris commented Feb 21, 2017 •
edited
Loading

Right, I looked in the wrong place. However, *geqrf appears in both single and double precision, so I think the intent was that all included lapack routines should be available in both single and double versions. I'm really suspecting the fact that the single version of *gelsd did not get used was an oversight. Note that those functions were included previously.

@charris
Copy link
Member
charris commented Feb 21, 2017 •
edited
Loading

Note that rebuilding and (possibly) stripping out functions should really have been two separate PRs.

@eric-wieser
Copy link
Member Author

Right - the reason the PR is the way it is is that I built it as it was, then added functions until I stopped getting errors. Perhaps I should have grepped the source code for function prototypes, but I think then I'd end up listing subroutines I didn't care about

@charris
Copy link
Member
charris commented Feb 21, 2017

Well, least squares works for single, but I'll bet it is now computing in double. Note that the addition of single functionality is fairly new, so something may have been easily overlooked.

@eric-wieser
Copy link
Member Author
eric-wieser commented Feb 21, 2017 •
edited
Loading

Assuming we're talking about the python code, the only decision it uses to pick between the functions is isComplex. Unless there's some type-specific dispatch somewhere that I haven't seen, it never checks which function is appropriate for single/double

@charris
Copy link
Member
charris commented Feb 21, 2017

The generalized ufuncs are defined in umath_linalg.c.src, and indeed the single versions of least squares are not included, but most everything else has both single and double versions. That may have been intentional for accuracy reasons, but maybe not. @pv Do you recall if this was an error or a deliberate decision.

@eric-wieser
Copy link
Member Author

Right, that jogs my memory - I only kept the functions that had prototypes declared in umath_linalg, since the others didn't seem to be exposed anyway

@charris
Copy link
Member
charris commented Feb 21, 2017

Hmm, OK, lstsq is not defined as a generalize ufunc in any case (see end of file) so the logic is probably in the python code and may well not allow for single.

@eric-wieser
Copy link
Member Author

Yep, that was the logic that I was alluding to earlier. I think you're right, we'll probably want these functions back in - but right now, they were presumably optimized out by the linker anyway. Putting them back in would come alongside adding some higher-level code to use them.

But now that the build tools work again, that's easy, right? ;)

@charris
Copy link
Member
charris commented Feb 21, 2017

So we aren't missing anything ... yet. As to how easy it is, I'll take your word for it :)

eric-wieser added a commit that referenced this pull request Feb 21, 2017
@eric-wieser
DOC: Fix obsolete data in readme
a1341d9 
Link was dead, and this project is neither Numeric nor in CVS any more.
This should have gone into #8381
This was referenced Feb 21, 2017
Merged
Merged
Closed
@eric-wieser eric-wieser mentioned this pull request Mar 7, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
03 - Maintenance component: numpy.linalg
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eric-wieser @rgommers @charris
0