Repository navigation
Time to release a new version? #98
Description
Activity
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).
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.
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-openblasCMake 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_blasDo 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.eigvalsisn't good enough.It doesn't look like we need matplotlib (which, at least on Linux, depends on Qt), though.
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 installwithpip 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 havepyproject.toml, right?). Anyway, for now I can at least do a basic build.a
git clean -fxdremovedsetup.cfgand_skbuild(and other odds and ends) and nowpip install .works;conda buildstill fails with the same error.conda build -c conda-forge --python=3.8 slycot/conda-recipe-openblasCMake 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 thebuild.shshould take care of that. In Travis, openblas and other build dependencies are installed explicitly before runningconda build. Maybe something wrong with the dependency declaration inmeta.yaml?@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 removingmathematical.pyffromCMakeLists.txt. Should be easy to resolve if git does not resolve the conflicts automatically.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.Ah, I forgot about running the python-control test suite as part of the slycot tests, thanks.
Please use the branches bnavigator/pr* for the rebase and merge as discussed in #84 (comment)
Another update: Because of the changes (and possibly more to come) in #88, The branches for #96 (not lytex/master but bnavigator/pr96-lytex-comples) and #97 (both bnavigator/travis-to-control-pytest and bnavigator/py97-pytest-travis-coveralls) are based on current master again. They should apply cleanly with or without #88.
I'm checking README.rst; it's mostly still OK.
From the Travis build matrix, Slycot is still passing tests on Python 2.7, so it's probably fine to leave it in for this release (I don't think 2.7 support is adding much burden to us?). Python 3.5 is EOL September 2020, (https://devguide.python.org/#status-of-python-branches), though I don't know if we've tested on that.
I don't know about minimum versions for other packages (numpy, scikit-build, cmake). We now need scipy, but only for a test, as I understand; I'll add that.
This needs updating:
You can also use conda to build and install Slycot from source. :: conda build conda-recipe conda install --use-local slycotconda-recipedoesn't exist anymore. I don't know which of the three recipes to recommend, though;- Presumably
conda-recipe-appleis only for macOS -- is it the preferred or only option there? - For Windows are both MKL and OpenBLAS working?
- For Linux the MKL / OpenBLAS choice is up to the user (both pass on Travis)
At the end we say you can install "plain old" LAPACK from conda; do we have a conda recipe supporting that? If not, should we just drop that paragraph, or is it intended for "python setup.py install" run inside a conda environment with LAPACK installed---in which case, perhaps its worth stating that in full.
- Presumably
81 remaining items
E Failed: Example ab13fd_example produced a warning. E Captured output: E The stability radius is E 0.003919647231698771 E The minimizing omega is E 0.9896652038894939 E E Captured stderr: E E Captured warnings: E [SlycotResultWarning('\nFailed to compute beta(A) within the specified tolerance.\nNevertheless, the returned value is an upper bound on beta(A);')]Allowing it in af98e9f
I've at last released v0.4.0.
@moorepants are you (still?) a maintainer for slycot on PyPI? If so, could you please upload it?
@murrayrm I've at last created a PyPI account, my username is roryyorke.
- upload to PyPI
- file PR with conda-forge feedstock
It looks like I do have permission to upload. I assume you want me to:
git checkout v0.4.0 python setup.py sdist twine upload dist/slycot-0.4.0.0.tar.gzNote too that the conda forge pot will create a PR once PyPi is updated.
@roryyorke: Added you as a maintainer on PyPi.
@moorepants yes please.
I pushed it up.
🎉 yay!
For the next update (probably 0.4.1), could we please drop the fourth digit in the PyPI version? The git tag, the report during the CMake build and the .egg-info all report 0.4.0, but PyPI has 0.4.0.0.
Conda forge package is in.
I suggest submitting a PR to the feedstock then. I am not familiar with the details of how the tests have changed.
This is the command that runs on the feedstock:
python -c "from slycot import test; test()"If that doesn't run anymore, that seems like a backwards incompatibility.
I've sent a release notice to python-control-announce, so I think this is all done.
Thanks everyone!
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.