-
Notifications
You must be signed in to change notification settings - Fork 43
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
Time to release a new version? #98
Comments
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). |
So, somewhat embarrasingly, I can't build slycot. This is my build script:
CMake doesn't find BLAS:
I see this line in .travis.yml:
which I'll try to add now. While looking at that, I see we install scipy and matplotlib for slycot CI:
Do we need them?
That example needs generalized eigenvalues, so It doesn't look like we need matplotlib (which, at least on Linux, depends on Qt), though. |
This script suceeds:
Replacing
We don't currently give |
a |
Hm, setting the |
They are all based on master. Some might create minor merge conflicts, because they have the same change like removing the |
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 |
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:
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. |
On Python2: I'd say we leave it in as long as the CI builds it fine. |
Travis CI configuration in #97 builds both 2.7 and 3.5 versions. On the conda-recipes @repagh should have a word, as he knows what is going on in conda-forge feedstock. MKL is conda only. IIRC, "Generic" i.e. "plain old" LAPACK did not work in conda at all when I made the reorganization in #90 and #93. So unless this is different now, we should remove that paragraph at the end.
|
New featuresAdded periodic Schur decomposition functions
|
I would also use 0.4.0. We added some functionality, not only bug-fixes. https://semver.org/ |
Just catching up on this. Bumping the version number to 0.4.0 sounds fine. I checked out the branch for PR #101 and tried on my Mac. Building (and installing) via This could be something on my end, so let me do some work to make sure that this is reproducible. |
I guess it is during a unit test? Can you tell which one? Also, what about version 0.3.5? |
Confirming that I am able to successfully compile and run tests for The core dump happens if I just run I'm also going to try using the conda build recipe and see if that does anything different. |
I also tried using
|
Yep, looks like it doesn't find the MacOSX SDK. Did you check the travis configuration how it is handled there? |
Normally (and for the case of v0.3.5), I can just use the standard build process (via Let me do some debugging and see if I can find out what is causing the core dump. I've never seen that happen before, so may take a while. I'll also test on MacOS 10.14 (Mojave) and make sure it isn't something having to do with 10.15 (Catalina). |
Just as a hint for your debugging: I have seen coredumps building Slycot on Linux and the reason was often, that the wrong LAPACK/BLAS implementation was used. I think it has to be the same as Numpy is linked to on the system. |
@repagh added an update to #98 (comment) saying he could build, if I read correctly. I'll wait for him to respond. |
I tried the following:
When I run Output from otools:
I suspect the problem is that I am not getting a compatible version of compiler and math libraries? Here is the output of
As before, I don't think this is a blocker for releasing v0.4.0 and worst case I can always do a conda install of the conda-forge version. It's still odd, though. |
One thing is that you are installing a mix of conda packages from defaults and conda forge. I think its best to configure conda to install everything from conda forge. See this: https://conda-forge.org/docs/user/tipsandtricks.html#how-to-fix-it and https://docs.conda.io/projects/conda/en/latest/user-guide/tasks/manage-channels.html. |
Maybe using |
Loading everything from condo-forge seems to work. I now get things to compile (!). Running
Running
|
This diff turns warnings into errors, and lets one see the warning (see pytest docs) diff --git a/slycot/tests/test_examples.py b/slycot/tests/test_examples.py
index b926e2e..bb9cdae 100644
--- a/slycot/tests/test_examples.py
+++ b/slycot/tests/test_examples.py
@@ -13,9 +13,10 @@ from slycot import examples
examplefunctions = [fun for (fname, fun) in getmembers(examples)
if isfunction(fun) and "_example" in fname]
+pytestmark = pytest.mark.filterwarnings("error")
@pytest.mark.parametrize('examplefun', examplefunctions)
-def test_example(examplefun, capsys, recwarn):
+def test_example(examplefun, capsys):
"""
Test the examples.
@@ -26,4 +27,3 @@ def test_example(examplefun, capsys, recwarn):
captured = capsys.readouterr()
assert len(captured.out) > 0
assert not captured.err
- assert not recwarn I tested this in an editable install (
So, if I read that right, it's not only on macOS that Slycot gets its numpy libraries mixed up. This doesn't happen with Finally, if I run
|
That ufunc size warning is triggered by the In our case, we can work around it by importing scipy.linalg at module level in test_examples.py (I don't know why this works; perhaps it is pytest resetting import filters, as suggest in a comment in [2].) [1] numpy/numpy@170ed4e |
That is just maddening. I |
Deprecated command, wich does not use pytest. Error looks like an old slycot |
@murrayrm, could you please check with #133, which failure in the examples that would be? |
Ran #133 and got the following output from
|
Interesting. Should we increase the default tolerance in the example or introduce an option to ignore specific warnings in examples? |
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. |
|
It looks like I do have permission to upload. I assume you want me to:
|
Note 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:
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.
The text was updated successfully, but these errors were encountered: