-
Notifications
You must be signed in to change notification settings - Fork 106
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
branch off new slycot version #1689
Conversation
Will the current workaround break with 0.5.0? |
No, it shouldn't.
That might be better, to support more Slycot versions. Or we could postpone merging this PR until there are a few more releases of Slycot, in a year or two. |
Great, so there's no rush to get this in before the release.
I would prefer the version branch for now. We can add a deprecation warning for That sound good @artpelling, @pmli ? |
Yes sounds good, will get on that soon. This really is low priority, everything should continue to work fine even if we do nothing. :) |
Maybe you could also look at the warnings we get from slycot when you get to this? |
57ef438
to
395da13
Compare
@renefritze I just wanted to test the changes locally, but all the lyapunov related tests are skipped, because I don't have pymess. The decorators
make it such that the tests are skipped if either package is missing. What would be the best way to change this behaviour? Should I conditionally edit the solver lists in pymortests/lyapunov.py? For now, I worked around it and tests pass for all slycot versions. |
Sorry @artpelling, I tried to contact you on gitter. |
@lbalicki might know what to do about the issue with the
|
Just fyi: if you rebase on main, the newer slycot version will also be included in the CI images for gitlab CI. |
509a9fc
to
57ac9e0
Compare
I guess this can be merged now? Do we have some workflow for reminding ourselves to add deprecation warnings for Slycot in |
Let's check the log in a bit if this works in Gitlab too.
Not explicitly. Just create a new issue and add that to the release milestone? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logs look good. Thanks Art!
Codecov Report
Additional details and impacted files
|
I saw that a new slycot version was released last week. It includes my bugfix PR for an incorrectly set
ldwork
variable in the discrete-time case. If we update our requirements to version 0.5.0 or higher, we can remove the workaround bugfix from our bindings.The new release does not seem to contain drastic changes, although they have switched to a different license and version of SLICOT and there seems to be an issue with NumPy 1.23.0 (but not 1.23.1 and greater).
https://github.com/python-control/Slycot/releases/tag/v0.5.0