Time to release a new version?

[bnavigator]

Hi,

with all the merged and pending PRs that would be ready to merge, I think it is warranted to take Slycot to the next release. For example, further development of python-control/python-control#376 would need Slycot's support of complex matrices.

@roryyorke are you still around? Any other maintainer? I am also willing to help.

[murrayrm]

I can do the merges and release if needed, but it would be much better for @roryyorke and/or @repagh to take a look since they are the people maintaining slycot (and figuring out how to make sure it builds in conda-forge, which seems to be quite non-trivial).

[roryyorke]

There are a number of outstanding PRs. I'd ask for @bnavigator's help to review them, but they're almost all his :).

I see #73 has a number of unaddressed remarks - shall we defer this? I don't know if python-control needs mb03rd. @repagh , do you want to take a look at this?

#80 looks experimental, and related to CI; I think we can defer this too.

If the above is right, the PRs to merge are:

• fix invalid variable usages from wrapper outputs #79
• Fix test sg03ad #82
• Fix td04ad_case1 and td04ad_static tests #83
• deal with python-control#347: missing exception attribute #84
• sg03ad parameter N is not hidden by the wrapper #87
• add schur decomposition wrappers #88
• add ab08nz function to support complex state-space systems #96
• switch the additional python-control unittests to pytest in Travis #97
• Update README.rst build instructions #101
• map logical to 1 or 0 independent of compiler in mc01td #103
• run examples in testsuite #104
• clean test files #105
• Fix unit tests option 2: run pytest with --pyargs slycot by default #110
• limit line lengths to 72 char for fortran compiler compatibility #112
• Update cmake files #114
• Run tests on conda build #115
• Add mb03rd wrapper: schur to block-diagonal transform #116

It looks like #79 is pretty much good to go, I'll start with a quick review there, and merge it in,~~

@bnavigator , do you know if all of these work together? Do you have a branch where they're all merged in?

We can review the issues (most look like the usual build-difficulty) after we merge these.

[roryyorke]

So, somewhat embarrasingly, I can't build slycot. This is my build script:

#!/bin/sh
set -ex
export PAGER=

(cd slycot && git log -1)
(cd slycot && git status)

conda --version
conda build --version

conda build -c conda-forge --python=3.8 slycot/conda-recipe-openblas

CMake doesn't find BLAS:

  CMake Error at /home/rory/.miniconda3/conda-bld/slycot_1586503830485/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_plac/share/cmake-3.17/Modules/FindPackageHandleStandardArgs.cmake:164 (message):
    Could NOT find BLAS (missing: BLAS_LIBRARIES)

I see this line in .travis.yml:

      export LIBRARY_PATH="$HOME/miniconda/envs/test-environment/lib"

which I'll try to add now.

While looking at that, I see we install scipy and matplotlib for slycot CI:

      conda create -q -n test-environment \
            python="$SLYCOT_PYTHON_VERSION" \
            pip coverage pytest-cov \
            numpy scipy matplotlib \
            $conda_blas

Do we need them?

rory@rory-latitude:~/src/slycot/slycot$ grep -r scipy *
examples.py:    from scipy.linalg import eigvals
tests/test_sg02ad.py:        # https://github.com/scipy/scipy/issues/2251
rory@rory-latitude:~/src/slycot/slycot$ grep -r matplotlib *

That example needs generalized eigenvalues, so numpy.linalg.eigvals isn't good enough.

It doesn't look like we need matplotlib (which, at least on Linux, depends on Qt), though.

[roryyorke]

This script suceeds:

#!/bin/sh
set -ex
export PAGER=

(cd slycot && git log -1)
(cd slycot && git status)

python3 -m venv --clear venv-build-slycot
source venv-build-slycot/bin/activate
pip install -U pip
pip install scikit-build cmake numpy
(cd slycot && python setup.py install)
python -c "import slycot; print(slycot._wrapper)"

Replacing python setup.py install with pip install . fails, apparently here:

  CMake Error: The source "/tmp/pip-req-build-dmsubg0d/CMakeLists.txt" does not match the source "/home/rory/src/slycot/CMakeLists.txt" used to generate cache.  Re-run cmake with a different source directory.
    File "/tmp/pip-build-env-w1oj5yw4/overlay/lib/python3.6/site-packages/skbuild/setuptools_wrap.py", line 574, in setup
      languages=cmake_languages
    File "/tmp/pip-build-env-w1oj5yw4/overlay/lib/python3.6/site-packages/skbuild/cmaker.py", line 232, in configure
      os.path.abspath(CMAKE_BUILD_DIR())))

We don't currently give pip install . as an installation method in README.rst, though it would be nice if it was supported (that's why we have pyproject.toml, right?). Anyway, for now I can at least do a basic build.

[roryyorke]

a git clean -fxd removed setup.cfg and _skbuild (and other odds and ends) and now pip install . works; conda build still fails with the same error.

[bnavigator]

conda build -c conda-forge --python=3.8 slycot/conda-recipe-openblas

CMake doesn't find BLAS:

  CMake Error at /home/rory/.miniconda3/conda-bld/slycot_1586503830485/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_plac/share/cmake-3.17/Modules/FindPackageHandleStandardArgs.cmake:164 (message):
    Could NOT find BLAS (missing: BLAS_LIBRARIES)

Hm, setting the BLA_VENDOR="OpenBLAS" in the build.sh should take care of that. In Travis, openblas and other build dependencies are installed explicitly before running conda build. Maybe something wrong with the dependency declaration in meta.yaml?

[bnavigator]

@bnavigator , do you know if all of these work together? Do you have a branch where they're all merged in?

They are all based on master. Some might create minor merge conflicts, because they have the same change like removing the suite() lines in test files and removing mathematical.pyf from CMakeLists.txt. Should be easy to resolve if git does not resolve the conflicts automatically.

[bnavigator]

While looking at that, I see we install scipy and matplotlib for slycot CI:

[...]

Do we need them?

Scipy wasn't needed by Slycot's own unit tests until now. The current 0.3.5 OpenSUSE package builds without it. However, #96 introduces the use of scipy.linalg.eig, too. The python-control tests that we run in Travis CI definitely need it. Matplotlib (probably) too.

[roryyorke]

Ah, I forgot about running the python-control test suite as part of the slycot tests, thanks.

[bnavigator]

In https://github.com/roryyorke/Slycot/tree/rory/rebase-merge, the commits 0ded75f and 46769a9 are duplicates of f6494b8 and 91596c1. They belong to #88. but come between #96 and #97.

And f4c4277 belongs to #96 but comes after the commit 158c772 for #97

[bnavigator]

Please use the branches bnavigator/pr* for the rebase and merge as discussed in #84 (comment)