-
-
Notifications
You must be signed in to change notification settings - Fork 10.9k
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.
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
Conversation
Hmm, those tests were cancelled for some reason. |
a40ebcb
to
4d11649
Compare
@pv @ovillellas: Don't suppose you remember how you built these files in c679f7b? Seems that somehow |
39bcf29
to
f97355f
Compare
This latest build strips out |
Nope, someone had manually added an |
Awesome, that actually worked! Ready for review, I think. |
The last paragraphy of README says:
The diff in this PR doesn't have these additional parentheses. |
It would also be useful to add some instructions to README on how to test |
Correct. Manually obtaining the f2c source, patching without instruction, and rebuilding seems like a lot of busywork to fix unimportant warnings. |
@rgommers: Are you content for me to just remove these lines from the README? |
f97355f
to
e7dcd52
Compare
There's nothing really to say here. You test the generated files by building the generated files, then running the numpy test suite. |
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
This is also defined in python_xerbla.c, where we redefine a python-compatible version
These are taken from lapack_litemodule.c and umath_linalg.c.src
e7dcd52
to
224abf8
Compare
What is the motivation for this, what does it fix/add? |
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". |
@charris: The underlying issue requiring an upgrade of lapack-lite is discussed here |
@eric-wieser The way to test this is to force a build that uses lapack_lite. For instance
will not use any system libraries. |
@charris: This is already tested by appveyor, I believe. (Since it failed until I got it right) |
You should be able to define those variables in the |
But adding it explicitly to travis seems like a good idea anyway, so I'll do that too |
Yep, better done on purpose than by accident. |
@charris: Alright, done and passing |
Let's get this underway. Thanks Eric. |
About stripping out |
But right now, |
Right, I looked in the wrong place. However, |
Note that rebuilding and (possibly) stripping out functions should really have been two separate PRs. |
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 |
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. |
Assuming we're talking about the python code, the only decision it uses to pick between the functions is |
The generalized ufuncs are defined in |
Right, that jogs my memory - I only kept the functions that had prototypes declared in |
Hmm, OK, |
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? ;) |
So we aren't missing anything ... yet. As to how easy it is, I'll take your word for it :) |
Link was dead, and this project is neither Numeric nor in CVS any more. This should have gone into #8381
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:
f2c
, but that patch was not recorded, and patchingf2c
as part of the build process is a lot of effort just to squelch some warnings