Update SLICOT to 5.7 with BSD license

[bnavigator]

This is the first take on updating our SLICOT 5.0 GPL source to the official BSD-3-Clause licensed SLICOT-Reference 5.7. See also #146.

Currently, the new source is a submodule with path "../SLICOT-Reference", referencing my fork on bnavigator/SLICOT-Reference. I don't have permission to fork into python-control/SLICOT-Reference. Unless the path is hardcoded to bnavigator, merging this PR will not result in a working Slycot then.

Some fixes to obsoleted and removed LAPACK routines, which we (mainly @repagh) already have applied to our SLICOT 5.0 sources, needed to be ported to SLICOT 5.7. See https://github.com/bnavigator/SLICOT-Reference/tree/slycot-changes. I can submit my own work upstream. But unless stated otherwise by the author himself, every contribution to Slycot so far is under GPL. So you need to submit your fixes to upstream yourself, if you wish to do so, @repagh.

[murrayrm]

Thanks for taking a first cut at this! Is there a reason to create the submodule using a reference to the python-control project (or your fork, for now) rather than direct to the SLICOT project (SLICTO/SLICOT-Reference)?

[bnavigator]

rather than direct to the SLICOT project (SLICTO/SLICOT-Reference)?

Yes, we need some changes to the sources so that we can build on gcc10 and newer LAPACK.

[bnavigator]

Fails in the python-control test suite, because it has better coverage than the Slycot tests: Some call signatures have changed!

First one fixed is sb03md, which indeed had inconsistencies in the call signatures between the Python function, the pyf wrapper and the Fortran routine.

[roryyorke]

I've also taken a crack at this, see https://github.com/roryyorke/Slycot/tree/rory/slicot-5.7. That builds fine on Ubuntu 20.04 using conda-provided Python 3.8, numpy 1.19.5, gcc_linux-64 9.3.0, gfortran_linux-64 9.3.0, libblas 3.9.0, liblapack 3.9.0, libopenblas 0.3.12. After I cherry-picked your sb03md signature changes, all Slycot and 1502d38 tests pass.

I presume if activate Github actions on that repo, I can run tests on Windows and Mac too; I haven't explored that.

I didn't use submodules, but that is not my main point: I don't know the trade-offs between sub-modules and "other" options. What I do want to note is below.

1. I started by building SLICOT with Ubuntu-provided GNU make gfortran (called as f77), and in order to get SLICOT to build I needed the diff below; I haven't tried building Slycot with this diff taken out:

modified   examples/TMB03LD.f
@@ -28,7 +28,8 @@
      $                   Q( LDQ, 2*NMAX )
 *
 *     .. External Subroutines ..
-      EXTERNAL           MB03LD
+      EXTERNAL           MB03LD,  LSAME
+      LOGICAL            LSAME
 *
 *     .. Intrinsic Functions ..
       INTRINSIC          MAX

2. I have not needed the *LATZM and DGEGS -> DGESS (or whatever) changes, both when building with SLICOT with Ubuntu gfortran/lapack/openblas, and when building Slycot with conda gfortran/lapack/openblas; this surprised me. Does anyone remember on what platform we did need this for?

3. I see one somewhat scary warning during build; it looks real (I think Fortran-arrays indices are 1-based):

[722/1200] Building Fortran object slycot/CMakeFiles/_wrapper.dir/src/SLICOT/MA02ID.f.o
../../../slycot/src/SLICOT/MA02ID.f:174:21:

  170 |          DO 90 J = 1, N
      |                                                                        2
......
  174 |                DWORK(J-1) = DWORK(J-1) + TEMP
      |                     1
Warning: Array reference at (1) out of bounds (0 < 1) in loop beginning at (2)
../../../slycot/src/SLICOT/MA02ID.f:174:34:

  170 |          DO 90 J = 1, N
      |                                                                        2
......
  174 |                DWORK(J-1) = DWORK(J-1) + TEMP
      |                                  1

[bnavigator]

3. I see one somewhat scary warning during build; it looks real (I think Fortran-arrays indices are 1-based):

See #111

[bnavigator]

2. I have not needed the *LATZM and DGEGS -> DGESS (or whatever) changes

#13. I think I remember that my system spit out errors without the changes.

Testing without it: https://github.com/bnavigator/Slycot/actions/runs/565931777

[roryyorke]

I missed the AB13ID (7b96b647) fix from upstream; the annoyance in getting this into my branch has put me in favour of the submodule approach.

If we do go the submodule route, could we briefly document how we go about updating SLICOT, either in README.md, or maybe SLICOT.md?

My guess is it would work something like this:

• we fork SLICOT-Reference to python-control/SLICOT-Reference
• in that repo we have a branch "upstream" that is a manually-updated mirror of upstream
• we also have the usual "main" branch, which has any Slycot-specific changes
• we occasionally merge "upstream" to "main"

[bnavigator]

My guess is it would work something like this:

• we fork SLICOT-Reference to python-control/SLICOT-Reference
• in that repo we have a branch "upstream" that is a manually-updated mirror of upstream
• we also have the usual "main" branch, which has any Slycot-specific changes
• we occasionally merge "upstream" to "main"

Basically, what I did in bnavigator/SLICOT-Reference:

• Keep main -> upstream/main mapping
• Use slycot-changes
• Fast-Forward upstream/main into main and merge it into slycot-changes as needed.

Submodules work on commit references, which can point into any branch. This reference is saved for the respective commit in python-control/Slycot or any forked repository.

[bnavigator]

Testing without it: https://github.com/bnavigator/Slycot/actions/runs/565931777

Tests were successful. I wonder if just my memory plays me a trick, or it maybe was just because of a failing intermediate test run between the merged changes from #13 and bnavigator/SLICOT-Reference@24a5d2d.

So do we ditch the modifications, or do we still need it? Those functions are deprecated, albeit still present.

[roryyorke]

3. I see one somewhat scary warning during build; it looks real (I think Fortran-arrays indices are 1-based):

See #111

Ugh, thanks.

#13. I think I remember that my system spit out errors without the changes.

I can't figure out how to install (now ancient?) conda-forge lapack 3.6.0 to test what happened back then. I see on scipy/scipy#5266 there is mention of being able to force LAPACK to be built with these deprecated functions in---I wonder if that is what is being done? I also see in #13 that @repagh tried to use the recommended replacement for *LATZM, but something went wrong.

Sorry for the noise---I don't want to make extra work, I was just surprised when SLICOT compiled without complaint. I guess it would be safest to keep our SLCT_*LATZM functions. The other change (DGEGS) is more straightforward, and a good candidate for sending upstream (assuming licensing can be ironed out).

[roryyorke]

So do we ditch the modifications, or do we still need it? Those functions are deprecated, albeit still present.

I reckon we should keep our modifications for the deprecated LAPACK functions.

[bnavigator]

Upstream has their own modifications between 5.0 and 5.7 regarding DGEGS/DGGES. I am pretty sure I ran into errors here, but as I said, that was maybe because something between backported #13 and not yet applied bnavigator/SLICOT-Reference@24a5d2d. Not applying any of those also seems to work.

[roryyorke]

Thanks @bnavigator, this is great. To wrap this up, I think we need to:

1. fork your SLICOT-Reference to a new repo under python-control, and point the submodule to that - @murrayrm , do you agree?
2. Update README.md to explain how to initialise the module, along with a pointer to https://git-scm.com/book/en/v2/Git-Tools-Submodules
3. Also update README.md with an explanation of our update policy w.r.t. SLICOT (merge upstream/main to main & merge that to slycot-changes; Slycot-specific changes to slycot-changes)

I'm happy to do the README.md updates; let me know.

[murrayrm]

Sounds like a good plan to me. Anything special needed for SLICOT-Reference? Otherwise, I can create the repository, set @bnavigator, @roryyorke, @repagh as owners, and then let you at it.

[bnavigator]

Sounds good. I suggest forking from SLICOT/SLICOT-Reference, so that we have the correct link registered by GitHub.

[murrayrm]

SLICOT/SLICOT-Reference forked. @bnavigator, @roryyorke, @repagh invited as maintainers.

[bnavigator]

• I pushed the slycot-changes branch to python-control/SLICOT-Reference.
• The submodule reference is updated
• New instructions in Slycot README how to get the full source code

[bnavigator]

2. Update README.md to explain how to initialise the module, along with a pointer to https://git-scm.com/book/en/v2/Git-Tools-Submodules
3. Also update README.md with an explanation of our update policy w.r.t. SLICOT (merge upstream/main to main & merge that to slycot-changes; Slycot-specific changes to slycot-changes)

I'm happy to do the README.md updates; let me know.

I took the first steps, please feel free to enhance.

[roryyorke]

Excellent, let's merge it.