MAINT: Split lapack_lite more logically across files #8651
Conversation
|
Can you document up top what you did in more detail? There are are a bunch of new files, what is in them? The |
Perhaps, but only I'll have another go at writing the description |
|
Also, the current files are divided into complex and real, any reason not to just rename the files We probably need to revise the HAVE_CONFIG stuff, IIRC, it just treats long double, which does not apply here. |
You're referring to master, or this PR? Master is divided into complex128 and everything else I think having two 30k line files is probably more manageable than one 60k line file, so I'd argue for
I've updated the description |
|
I'd be happy with four |
Do you want And what about the stuff that doesn't fit into any of those categories?
I'm hoping that the diffs should be smaller when the only changes are actually an upgrade, rather than moving lots of content around. But I guess if you're happy with the 4-way split, I can go ahead locally and verify that the resulting diff is indeed small.
I'll give this another pass tomorrow. Thanks for the feedback! |
|
Ok, hoping the up-top description is more useful now |
| { | ||
| /* System generated locals */ | ||
| - real ret_val; | ||
| + volatile real ret_val; |
There was a problem hiding this comment.
This was forgotten in the previous version, so was presumably causing the same bug as the code that the other part of this patch fixed.
|
This looks much nicer. I guess the only question is the naming/merging of I can see that I'm not much help here ;) Do you have any preferences? |
|
Maybe |
|
I'm coming around to just renaming |
|
Perhaps I have no preference for any of these names. The key part of this was splitting |
|
|
I don't think I agree. The current |
|
OK, I don't feel strongly about it. |
|
Also, is the |
Read my mind. The only reason for keeping it I could think of was that it isn't the complete lapack, but rather a "lite" version, as in Miller lite (TM) beer, so the suffix does make some sense. |
|
What I meant by that is is it needed on each file? I'm all for the folder having that name. |
|
Sure, go ahead and remove the "lite" suffix, I don't have strong feelings either way. |
ffb17f5 to
b2c6e2b
Compare
|
@charris: Done, and rebuilt the commits in slightly more logical order, and to emphasize the bug fix. All intermediate commits build, I think. I've tried to include the important bits of the issue text in the commit messages. The end choice was |
|
Oh, now, that is a step too far ;) I do prefer the |
If this is something we care about, then we need to only use 6 characters in the name too though, right? ;) Seriously though - do you want me to revert that, or not? |
|
I really do prefer the sdcz prefixes, as I instantly know what they mean, i.e., functions beginning with the same prefixes. |
|
How about |
|
Geez. How about |
|
Not as happy with I'll defer to you decision here, but I'd rather not rebase this more than once more. So can you confirm you're definitely sufficiently content with On a more positive note, I have #8649 just about working successfully on top of this now, although currently with a few hacks to avoid having to write fortran |
|
Yeah, I thought if the sorting issue and decided that with so few files it didn't matter much. You could always prepend |
Yeah, I agree with that
Actually, i think doing something like this is probably a good idea, to discourage editing. How do you feel about |
|
|
|
Although I think a straight |
|
Let's make a decision:
Let's make this the last time we pick names here :) |
|
I like |
|
I like |
Since we can use 2.7+ features now, we can have the with statement and subprocess.check_call
b2c6e2b to
66cc1c0
Compare
|
@charris: All done. Also took the chance to throw a little bit more cleanup into an extra first commit. |
| @@ -188,64 +201,61 @@ def getLapackRoutines(wrapped_routines, ignores, lapack_dir): | |||
| return library | |||
|
|
|||
| def getWrappedRoutineNames(wrapped_routines_file): | |||
| fo = open(wrapped_routines_file) | |||
There was a problem hiding this comment.
This was never closed before
66cc1c0 to
eae362f
Compare
Also uses this splitting as an excuse to ditch the _lite suffix, in favor of a f2c_ prefix for all generated files. Before: * `zlapack_lite.c` - Functions for the `complex128` type. * `dlapack_lite.c` - Every other lapack function After: * `f2c_z_lapack.c` - Functions for the `complex128` type. * `f2c_c_lapack.c` - Functions for the `complex64` type. * `f2c_d_lapack.c` - Functions for the `float64` type. * `f2c_s_lapack.c` - Functions for the `float32` type. * `f2c_lapack.c` - Every other lapack function
Previously, the dlamch function was a manually-edited file, precluding autogeneration. We fix this by putting the manual edits in a patch file, and then generating f2c_config.c from install/*.f, which includes dlamch. While these functions exist in more than once place in lapack 3.0.0, they only exist in lapack/install in newer versions. A side effect of this is that some functions have been pulled out of `f2c_blas.c` and `f2c_s_lapack.c` into this new file. The edits in the patch were introduced in cadbb5f, for a reason that is lost to time.
This bug is mentioned, fixed, but not described, in cadbb5f To be safe, we patch it for the equivalent float32 function.
eae362f to
9a77c2a
Compare
|
Also, the upside of us arguing about names is that by rebasing again, I remembered to fix the readme and comments to match the new names |
|
In it goes. Thanks Eric. |
Primary goals
<lapack>/INSTALLto the search path, since some fortran files are duplicated to here.Old structure
dlamch.c(manually generated, edited) - Thedlamchfunction. Includes the patch in cadbb5f1, so no longer auto-generatedzlapack_lite.c- Functions for thecomplex128type.dlapack_lite.c- Every other lapack function2New structure
f2c_z_lapack.c- Functions for thecomplex128(z) type.f2c_c_lapack.c- Functions for thecomplex64(c) type.f2c_d_lapack.c- Functions for thefloat64(d) type.f2c_s_lapack.c- Functions for thefloat32(s) type.f2c_config.c-dlamch,slamch(for which patchf2c_lapack.c- Every other lapack function.The patch from cadbb5f, which was forgotten, and any other function that comes from the
/INSTALLdirectory of lapack3 is included inf2c_config.c. Themake_lite.pyscript now applies thef2c_config.c.patchto the result, which means that this patch can't be forgotten in future1 Which unfortunately has no description of the bug it was trying to fix
2 Probably due to the build script not being correctly updated when the
candsvariants were added3 (Identical copies of)
[sd]lamch.fcan be found in both/INSTALLand/SRCwithin the LAPACK tarball. Previously, we were loading it fromSRC. However, in later releases of LAPACK it exists exclusively in/INSTALL. This addsINSTALLfirst on the function lookup path, and puts all the generated code from that path inmisc_lite.c. This makes the approach forwards-compatible.Questions
[sd]lamchspecial enough to get its own file like this, or can that go in the main[sd]lapack_.lite.cslamchless special thandlamchas indicated by the lack of manual generation, or is this a mistake made long agolapack_lite.candmisc_lite.c(or single name if they can be merged)