Skip to content
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

SLICOT 5.7 is on GitHub under BSD-3-Clause #146

Closed
pmli opened this issue Feb 2, 2021 · 13 comments · Fixed by #152
Closed

SLICOT 5.7 is on GitHub under BSD-3-Clause #146

pmli opened this issue Feb 2, 2021 · 13 comments · Fixed by #152

Comments

@pmli
Copy link

pmli commented Feb 2, 2021

The repository is here: https://github.com/SLICOT/SLICOT-Reference

Would there be interest in using it for the Slycot project?

I tried just copying the Fortran files and installing Slycot from source, but it seems that there are issues due to new SLICOT functions.

@bnavigator
Copy link
Collaborator

bnavigator commented Feb 2, 2021

Interesting. Is it legit? The github owners of SLICOT/SLICOT-Reference seem to be the original SLICOT authors. Ping @VasileSima4 @andreasvarga.

How is the relation to the GPL-licensed version 5.0, which we currently use, and to slicot.org and Niconet? They can just release their own software under a new license?

@bnavigator
Copy link
Collaborator

I tried just copying the Fortran files and installing Slycot from source, but it seems that there are issues due to new SLICOT functions.

Do you have error messages to post? Did you try to adjust slycot/CMakeLists.txt or the .pyf wrappers? Maybe we can discuss this in a new issue or PR.

@pmli
Copy link
Author

pmli commented Feb 3, 2021

As far as I'm aware, the SLICOT developers decided to switch to BSD for new releases. I guess it might be possible for Slycot to also switch (after the decision by Slycot developers, of course).

I tried just copying the Fortran files and installing Slycot from source, but it seems that there are issues due to new SLICOT functions.

Do you have error messages to post? Did you try to adjust slycot/CMakeLists.txt or the .pyf wrappers? Maybe we can discuss this in a new issue or PR.

I didn't attempt any fixes, I just wanted to see how far only copying the new version would go. Installing with python setup.py install worked on my Linux machine, but running the tests produced an error message about an undefined symbol mb01rb_, and MB01RB is one of the new functions. We could continue discussing this in a new issue or PR.

@bnavigator
Copy link
Collaborator

I guess it might be possible for Slycot to also switch (after the decision by Slycot developers, of course).

That might be difficult. The code base has quite some history with many contributors. We would need the permission to switch from GPL to BSD from every single contributor in the past.

But replacing the SLICOT Fortran sources is possible, AFAICT.

@ilayn
Copy link

ilayn commented Feb 3, 2021

That won't be necessary since the wrappers don't need to be GPL or I can rewrite them if needed. It is not too hard and also renders python-controls BSD-3 license moot when you have GPL parts in it. Actually that's one of the reasons why I don't install Slycot.

@bnavigator
Copy link
Collaborator

bnavigator commented Feb 3, 2021

@ilayn: I have a different understanding.

The slycot package and the python-controls package are two separate packages living next to each other in your installation. Why would the GPL package taint your BSD package there? python-control doesn't redistribute any GPL parts.

the wrappers don't need to be GPL or I can rewrite them if needed.

But the wrappers are GPL currently and all parts need to be "rewritten" without being derivative work from GPL licensed code, or all code authors needs to agree to a switch.

This also includes the docstrings of the wrapper functions.

@ilayn
Copy link

ilayn commented Feb 3, 2021

Because python-control emits error messages for certain functionalities are not available without Slycot. And also it doesn't mention that this will change the license. So if someone wants to use it under the impression that it is BSD3 they would be waiting for a surprise.

@roryyorke
Copy link
Collaborator

My thoughts:

  1. For the short-term (or medium-term, given the resources we have), I think we should keep Slycot more-or-less as-is, but upgrade to SLICOT 5.7. Can we try to change Slycot so that vendoring of SLICOT is a bit easier? I used to be able to do that sort of thing with Subversion using "vendor branches", but haven't tried with git. Do we even know what version of SLICOT we currently have? As I recall from the last time I looked, it wasn't the most recent GPL-licensed version.

  2. We have a few SLICOT fixes; we should try to contribute them back upstream if appropriate, and if we can get the fix authors to release their changes under a BSD license.

  3. On rewriting: Slycot is about 7k lines of implementation, and 1.9k lines of tests (this is naive, wc -l counting). While not infeasible to rewrite, it's also a fair effort.

  4. Somewhat off-topic for Slycot: Scipy could possibly use SLICOT directly now; @ilayn has already posted to scipy-dev about that. Besides the many control-specific functions, some useful things might be Riccati equation solvers (does Scipy currently use Sylvester equation solvers for this?) and modal decomposition (BUG: zpk2ss uses controller canonical form; this is ill-conditioned for systems that are more than order 5-10 scipy/scipy#5912, though I think a Jordan-form similarity transform might be more generally useful).

@ilayn
Copy link

ilayn commented Feb 6, 2021

About 4:

I wrote the riccati solvers via the qz route internally uses trsyl. https://github.com/scipy/scipy/blob/master/scipy/linalg/_solvers.py#L325
I can't judge how good they are but tested against CAREX and DAREX suite and seems like they are OK. But I would trust SLICOT more that I would trust myself.

I didn't write the Lyapunov solvers and they use the typical trsyl route but I can't say I am happy about them.

As far as I can tell from scipy.signal implementations, the model objects need massive rewrite. Thus I wanted to start the discussion in the mailing list though no response yet. But SLICOT would simplify greatly most of the feature implementations.

@bnavigator
Copy link
Collaborator

  1. For the short-term (or medium-term, given the resources we have), I think we should keep Slycot more-or-less as-is, but upgrade to SLICOT 5.7. Can we try to change Slycot so that vendoring of SLICOT is a bit easier? I used to be able to do that sort of thing with Subversion using "vendor branches", but haven't tried with git. Do we even know what version of SLICOT we currently have? As I recall from the last time I looked, it wasn't the most recent GPL-licensed version.

IIRC the comments in the Fortran sources say 5.0.
Making the src/ subdir a git submodule comes to mind.

  1. We have a few SLICOT fixes; we should try to contribute them back upstream if appropriate, and if we can get the fix authors to release their changes under a BSD license.

PRs against SLICOT/SLICOT-reference?

@ilayn
Copy link

ilayn commented Feb 7, 2021

By the way, here is the SciPy discussion in case you would like to chime in https://mail.python.org/pipermail/scipy-dev/2021-February/024522.html The start is two emails back which you can travel via previous thread button.

Current sentiment is to keep things as they are and scipy devs would point to python-control for specialized functionality in the future.

@murrayrm
Copy link
Member

murrayrm commented Feb 7, 2021

Great to see SLICOT coming out as BSD-3 and if we can sort out how to replace the FORTRAN sources in slycot with the latest BSD release (sounds possible), this would allow python-control to be much more useful to people who are OK with BSD-3 but not GPL. It will be interesting to see how #152 goes and whether everything works on all of the platforms.

@bnavigator
Copy link
Collaborator

First PR to SLICOT-Reference is online: SLICOT/SLICOT-Reference#1

@bnavigator bnavigator linked a pull request Feb 9, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants