Upgrade to Lapack lite 3.2.2 #8649

eric-wieser · 2017-02-21T16:06:51Z

Implements #8376. We pick the earliest acceptable version for the bug this was aiming to fix (dgelsd does not correctly compute iwork), because each new version throws more fortran features not supported by f2c at us.

This is fully functional~~, but a little hacky right now.~~

Not sure how best to structure the commits here. Unlike the last PR, these don't stand up by themselves - but I don't really want to stick a huge generated diff in the same place as hand-written code.

charris · 2017-02-21T20:03:05Z

Given the huge number of -Wparentheses warnings in the test, it would be worth disabling that warning for gcc, see http://stackoverflow.com/questions/3378560/how-to-disable-gcc-warnings-for-a-few-lines-of-code. That should be OK for generated code where human readability for review is less a factor. I'd suggest the same for other compilers, but don't know if they actually generate relevant warning by default. Or might be worth checking if there is a less stringent warning flag for the tests.

eric-wieser · 2017-02-21T20:07:18Z

Yep, previously we used a patched f2c that added the parentheses, but that patch was lost to time, and forcing numpy developers to recompile f2c is a bit mean.

This problem was actually present in #8381 - should have made more noise there!

I'll make a quick Pr for that at some point

charris · 2017-02-21T20:33:27Z

I didn't see it when I compiled on my machine with -Wall -Wstrict-prototypes. I don't think we should apply fixes throughout, but disabling the check for the generated code makes sense.

8000

eric-wieser · 2017-02-21T20:46:28Z

Hmm, I can't get travis to show me the warnings for that build either. Perhaps it only shows warnings if any errors occur? (there is a real error hiding in the logs here)

charris · 2017-02-21T21:05:59Z

I'm wondering if pip picks up CFLAGS. However, I see that you introduced a new warning in 564b7cb

numpy/core/src/multiarray/shape.c:708:39: warning: passing argument 1 of ‘check_and_adjust_axis’ from incompatible pointer type [-Wincompatible-pointer-types]

eric-wieser · 2017-03-02T00:40:47Z

numpy/linalg/lapack_lite/lapack.f.patch

+-            END DO
+C           DO I = M, 1, -1
+C              IF( A(I, J).NE.ZERO ) EXIT
+C           END DO


This is a pretty bad patch, that instead of actually rewriting this code in F77 style, just says "oh, we probably didn't need this anyway". The entire set of patches is rewriting this kind of loop.

There's a patch here that probably better fixes this, which I should probably copy here.

@eric-wieser - I was very surprised that this wouldn't just convert. Trying bit with f2c (as packaged by debian testing), the problem does not seem to be the do, enddo construction, but rather the exit statement. It should not be needed to do more than replace that with a goto <end-label>.

p.s. I wish we could just compile the fortran; gfortran is a very nice compiler...

I can certainly give that a go - thanks @mhvk

Well, if the other patch just works, then maybe that is easier. Anyway, let me know if there are issues; I'm old enough for most of my scientific code to have been written in fortran.

eric-wieser · 2017-03-03T02:24:32Z

There's a now-hidden conversation here about the contents of the .patch files, that I should draw attention to.

I'm pretty sure that the f2c_[dszc]_lapack.f.patch files should all be the same - any recommendations for avoiding duplicating this information? (or better yet, the duplicated hunk within f2c_lapack.f.patch)

charris · 2017-03-14T19:13:37Z

@eric-wieser What is the status of this?

eric-wieser · 2017-03-14T19:46:59Z

@charris: Fortran patches probably need work, and I haven't found the time to improve them. It's possible the bits of code in need of patching are just optimizations though, and that everything is working correctly right now.

Also, if you can thing of a tidy way to not have essentially the same patch file four times, and then one patch with the same hunk four times, that would be handy

eric-wieser · 2017-03-25T16:05:47Z

@charris: Patches are fixed as recommended by @mhvk

charris · 2017-03-25T18:24:44Z

numpy/linalg/lapack_lite/f2c_lapack.f.patch

+-               IF( A(I, J).NE.ZERO ) EXIT
+-            END DO
+               IF( A(I, J).NE.ZERO ) GOTO 10
+   10       END DO


I don't think this is correct for the EXIT, what you have here is equivalent to the C continue, whereas you want a break, i.e., GOTO the first statement after the END DO. See https://gcc.gnu.org/onlinedocs/gcc-3.4.4/g77/CYCLE-and-EXIT.html .

Looking at the generated code, I can confirm you're correct. Darn.

charris · 2017-03-25T18:27:00Z

I don't think the EXIT fixes are correct.

eric-wieser · 2017-03-25T18:50:53Z

Alarming that tests didn't catch that.

eric-wieser · 2017-03-25T18:53:33Z

Ok, I've added a pair of fixup commits that resolve that. If they look ok, then I'll rebase them in

charris · 2017-03-25T20:07:35Z

numpy/linalg/lapack_lite/README.rst

+The output C files in git use the LAPACK source from the LAPACK_ page, using
+version 3.2.2. Unfortunately, the newer version use newer and newer FORTRAN
+features that are increasingly not supported bt ``f2c``. As these are found,
+the patch files need to be changed


'will need'

charris · 2017-03-25T20:07:37Z

numpy/linalg/lapack_lite/README.rst

-released version of LAPACK available at the LAPACK_ page.
+The output C files in git use the LAPACK source from the LAPACK_ page, using
+version 3.2.2. Unfortunately, the newer version use newer and newer FORTRAN
+features that are increasingly not supported bt ``f2c``. As these are found,


'bt' <- 'by'. Maybe just one "newer"

So

"Unfortunately, newer versions use newer FORTRAN features ..."

charris · 2017-03-25T20:14:36Z

numpy/linalg/lapack_lite/f2c_config.c

@@ -107,7 +106,6 @@ doublereal dlamch_(char *cmach)


    if (first) {
-	first = FALSE_;


Any particular reason to move this?

Because the oracle that is f2c decided to. This is generated code beyond my control

charris · 2017-03-25T20:16:41Z

numpy/linalg/lapack_lite/f2c_config.c

@@ -239,7 +237,6 @@ doublereal dlamch_(char *cmach)


    if (first) {
-	first = FALSE_;


See above. I think it is easier to follow setting first here, i.e., execute only the first time.

And so on for the rest.

charris · 2017-03-25T20:21:17Z

numpy/linalg/lapack_lite/make_lite.py

@@ -23,7 +23,7 @@
 # Arguments to pass to f2c. You'll always want -A for ANSI C prototypes
 # Others of interest: -a to not make variables static by default
 #                     -C to check array subscripts
-F2C_ARGS = ['-A']
+F2C_ARGS = ['-A', '-Nx800']


What is the -N option?

Something to do with function table sizes. Without it, f2c would error asking for it to be provided. It might no longer be needed now that dlapack is actually just the d functions

mhvk · 2017-03-25T20:25:10Z

numpy/linalg/lapack_lite/f2c_c_lapack.f.patch

+-                        IF( V( LASTV, I ).NE.ZERO ) EXIT
+                        IF( V( LASTV, I ).NE.ZERO ) GO TO 35
+                      END DO
+   35                CONTINUE


I'd put the label on the next statement, but really, that is total nitpicking. This certainly works...

This is the whole point in CONTINUE though, right?

Did this because it makes the patch more self-contained

ok, that makes sense.

Ah, another old Fortran guy. I remember when the compiler was written in assembly, came on paper tape, and ran in several kilobytes of memory. Not only that, but a single human being could read the code and understand it.

Haven't used paper tape, but did use punch cards... But I fear my real early programming experience was in BASIC on a PET 2001... (plus 6205 assembly...)

Ah, different generation. The good thing about paper tape was that you could edit it with a razor blade and scotch tape 8-)

Anyway, I confess that the first computer I programmed for was one of the bigger ones in the country at that time, an IBM 360 with two megabytes of memory.

charris · 2017-03-25T20:26:24Z

Patches look good, couple of other comments. Why the big increase in size? More functions or more complex code?

eric-wieser · 2017-03-25T21:20:29Z

Why the big increase in size?

I think a lot of the documentation got better. Remember that we're looking at 10 years of development to LAPACK here

eric-wieser · 2017-03-25T21:56:01Z

Should I remove the author from the README and/or add myself?

I'm not too keen on putting my email address that visible on the easily-scrapable web, but I could put a github username in that file if it helps?

Readme is fixed, and fixup commits have been squashed

This doesn't yet actually generate the files, since they would cause the diff to balloon

These are copied from the libf2c source code, which is presumably where all the others came from

charris · 2017-03-25T23:25:17Z

LGTM. Could you add something to the 1.13 release notes?

charris · 2017-03-25T23:53:08Z

I think you could just add your name and a date. If anyone wants to find out more they can use git blame to track it back to the relevant commit.

charris · 2017-03-25T23:57:19Z

Or you could just leave it be. Pearu's name is still in some of the f2py files as an histori D215 cal artifact even though others have worked on the source since.

Also, update authors of lapack_lite [ci skip]

charris · 2017-03-26T15:41:54Z

In it goes. Thanks Eric.

eric-wieser added 51 - In progress component: numpy.linalg 03 - Maintenance labels Feb 21, 2017

eric-wieser mentioned this pull request Feb 21, 2017

TST: Fix various incorrect linalg tests #8369

Merged

eric-wieser mentioned this pull request Feb 26, 2017

MAINT: Split lapack_lite more logically across files #8651

Merged

eric-wieser force-pushed the lapack_lite-3.2.2 branch from d53a43d to fd08dc5 Compare March 2, 2017 00:31

eric-wieser commented Mar 2, 2017

View reviewed changes

eric-wieser force-pushed the lapack_lite-3.2.2 branch 2 times, most recently from 8e78d20 to 15bc60a Compare March 3, 2017 02:22

eric-wieser mentioned this pull request Mar 7, 2017

ENH: broadcast lstsq #8720

Open

eric-wieser force-pushed the lapack_lite-3.2.2 branch from 15bc60a to 29b9226 Compare March 25, 2017 16:03

eric-wieser removed the 51 - In progress label Mar 25, 2017

charris reviewed Mar 25, 2017

View reviewed changes

eric-wieser force-pushed the lapack_lite-3.2.2 branch from f1f6d21 to c75af4d Compare March 25, 2017 18:55

charris reviewed Mar 25, 2017

View reviewed changes

mhvk reviewed Mar 25, 2017

View reviewed changes

eric-wieser added 3 commits March 25, 2017 22:00

ENH: Add patch files to allow LAPACK 3.2.2 to be f2c'd

fd6962f

This doesn't yet actually generate the files, since they would cause the diff to balloon

ENH: Add more functions copied to f2c_lite.

febacb2

These are copied from the libf2c source code, which is presumably where all the others came from

ENH: Rebuild all of lapack_lite from 3.2.2

0b525a5

eric-wieser force-pushed the lapack_lite-3.2.2 branch from c75af4d to 0b525a5 Compare March 25, 2017 22:01

DOC: Add changelog entry for new lapack_lite

555e0e8

Also, update authors of lapack_lite [ci skip]

charris merged commit b1c1d7c into numpy:master Mar 26, 2017

homu mentioned this pull request Mar 26, 2017

ENH: Allow size=0 in numpy.random.choice, regardless of array #8717

Closed

eric-wieser mentioned this pull request Apr 4, 2017

Regenerate LAPACK_lite from a newer version of LAPACK #8376

Closed

		@@ -107,7 +106,6 @@ doublereal dlamch_(char *cmach)


		if (first) {
		first = FALSE_;

		@@ -239,7 +237,6 @@ doublereal dlamch_(char *cmach)


		if (first) {
		first = FALSE_;

Uh oh!

Upgrade to Lapack lite 3.2.2 #8649

Upgrade to Lapack lite 3.2.2 #8649

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!