Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

hinfsyn and h2syn fixes#100

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

roryyorke
Copy link
Contributor

Needsjgoppert/Slycot#4

Fixes for hinfsyn and h2syn; added trivial unit tests.

@slivingstonslivingston self-assigned thisAug 10, 2016
@slivingston
Copy link
Member

Can you rebase your changes onto the current tip of master branch, so as to use your updates to the Travis CI configuration (from#102)?

@roryyorke
Copy link
ContributorAuthor

I'll force push a rebased branch, but this won't pass without an up-to-date Slycot. The conda packages used on Travis were published by@cwrowley beforejgoppert/Slycot#4 was merged.

@slivingston
Copy link
Member

@cwrowley Might you update the packages onhttps://conda.anaconda.org/cwrowley soon?

Alternatives:

  1. we create a shared conda packages account; it appears thathttps://conda.anaconda.org/python-control is available.
  2. I create one under my name and post new packages there.
  3. I create new packages and send them to@cwrowley for uploading.
  4. Try to build SLICOT and Slycot from sources on every Travis CI job.

@murrayrm
Copy link
Member

I think that alternative 1 is the right long term answer, so that python-control can have multiple admins. I've created a python-control organization on anaconda.org and set up@cwrowley and@slivingston as admins (I think).

@slivingston
Copy link
Member

@roryyorke Can you rebase again? The current tip of master branch (commitcdd3e73) has an updated Travis CI configuration that uses the python-control account on Anaconda Cloud, where there are or will be a conda package of Slycot that includes your changes of sb10ad().

@cwrowley
Copy link
Contributor

@slivingston I'd like to update the slycot package on the new python-control account at anaconda.org, to include the changes to sb10ad from@roryyorke, but I am worried about creating conflicting versions. The latest version number (according to pypi and@jgoppert 's slycot repository, which I believe is the most up-to-date fork) is still 0.2.0, and that's the version currently on anaconda.org. I'm hesitant to make my own version number and upload it to anaconda.org, in case it might create conflicts with versions others might release down the road. Is it time to make a fork of slycot here at python-control, and call that the definitive version?

@murrayrm
Copy link
Member

I agree that this might be the time to pull a fork of slycot into python-control, so that we have a bit better control over the version that python-control builds on.

@slivingston
Copy link
Member

I agree about maintaining a fork of Slycot in the python-control organization. Given that the original seems to be abandoned and that both the original andjgoppert/Slycot lack issue trackers, the fork under python-control might become the main point of Slycot development.

@slivingston
Copy link
Member

I created it athttps://github.com/python-control/Slycot

Fixed call to sb10ad.  Needs fixed slycot, e.g., [1][1]roryyorke/Slycot@42cdeb8
Similar fix to hinfsyn: np -> np_, and identical test.
@roryyorke
Copy link
ContributorAuthor

Have rebased oncdd3e73 and pushed, but it seems I've jumped the gun a bit.

I gather rebase-and-force-push is the preferred workflow for python-control PRs? Seems to go against the normal advice of never rebasing published branches, but I guess with a small community it'll work fine.

@slivingston
Copy link
Member

My motivation in asking for you to rebase is to allow us to restart Travis CI jobs and observe test results after making changes to the built Slycot packages that are on Anaconda Cloud. It is not important when you do it because we can restart Travis CI jobs against the tip of this pull request at any time.

If avoiding force-pushing is desired, we could have achieved the same result by having you cherry-pick (i.e., apply identical changesets and commit messages from) the relevant commits that are on master branch.

@slivingston
Copy link
Member

Regarding workflows that might involve force-pushing, I recommend the practice of:

  1. except possibly for very short-lived, narrow-scope branches, all published branches in the main repository must never be rebased or otherwise have published tags or commits modified;
  2. branches that are part of pull requests can be freely rebased, modified, or squashed, depending on comments during review.

The ambition of item 2 is for the changes that are finally merged into the main repository to be in a good state, including which commit of the main repository provides the base for the branch of the pull request. Furthermore, pull requests often come through forks of the main repository, so the issue of someone basing a new branch on something that is rebased is not to be expected (unless someone makes a fork of a fork, for example).

Observe that reworking (e.g., rebasing or squashing) branches of pull requests is a common workflow with Git; e.g.,

All of that said, I am not aware that there are established guidelines for the python-control project. It is feasible to follow a policy of never rebasing or modifying commits under any circumstances, if that is preferred among contributors.

However, I want to emphasize item 1 above; the main branches of the project are not being modified after being published. We could make it more strict so that all branches of the main repository including temporary branches are never rebased nor modified.

@roryyorke
Copy link
ContributorAuthor

Thanks, I appreciate the detailed reply.

@roryyorke
Copy link
ContributorAuthor

Is there any reason not to make a Slycot 0.3.0 release? This branch rebases cleanly against1953399, but isn't much good without the Slycot fix:https://travis-ci.org/roryyorke/python-control/builds/179263142

@murrayrm
Copy link
Member

I agree that we should be able to update to Slycot 0.3.0. However, it should also be the case that this code should run without errors without Slycot being installed, since Slycot is optional for python-control. Can you put some code into your unit tests that checks for Slycot and checks for the appropriate error in that case. There should be some other unit test code of that sort in thetest/ directory.

@roryyorke
Copy link
ContributorAuthor

Both h2syn and hinfsyn already had the ImportError to ControlSlycot conversion, and the unit tests have the@unittest.skipIf(not slycot_check(), "slycot not installed") decorators.

Is there something else you had in mind?

The hinfsyn unit test doesn't currently fail because sb10ad is absent from slycot 0.2.0, but because of a bug in the f2py argument annotations for that function.

@coveralls
Copy link

coveralls commentedDec 24, 2016
edited
Loading

Coverage Status

Coverage increased (+1.005%) to 77.01% when pullingd923c6b on roryyorke:rory-robust-test intocdd3e73 on python-control:master.

@murrayrm
Copy link
Member

murrayrm commentedDec 24, 2016
edited
Loading

I've updatedslycot to 0.3.0 inslycot 3c7b91b and uploaded this version to anaconda. It looks like this is building correctly across python 2.7 and 3.4, but not 3.3 (deprecated, so doesn't matter) nor 3.5. From the logs, the error in the python3.5 build looks like something is wrong with conda, since slycot is not being found for some reason.

Will continue to debug until I can get the checks to pass. Hints welcome.

@murrayrm
Copy link
Member

The error in python 3.5 seems to be due to lapack not loading correctly. Specifically, I see the following error in the Travis CI log:

ERROR: testMIMO (control.tests.frd_test.TestFRD)----------------------------------------------------------------------Traceback (most recent call last):  File "/home/travis/build/python-control/python-control/control/xferfcn.py", line 1090, in _convertToTransferFunction    from slycot import tb04ad  File "/home/travis/miniconda/envs/test-environment/lib/python3.5/site-packages/slycot/__init__.py", line 16, in <module>    from .analysis import ab01nd,ab05md,ab05nd,ab07nd,ab08nd, \  File "/home/travis/miniconda/envs/test-environment/lib/python3.5/site-packages/slycot/analysis.py", line 21, in <module>from . import _wrapperImportError: liblapack.so.3: cannot open shared object file: No such file or directory

This is causingslycot to load but fail => the overall test fails.

@cwrowley: I might need to update the lapack package in conda as well. Can you provide some information in what is in that package?

@murrayrm
Copy link
Member

Found and fixed the problem in python3.5. For posterity: I had installed lapack on my local machine and so conda was picking up that version when it build slycot. Since that version was not present on the virtual machine used for Travis CI, it caused an error. I uploaded a new version of slycot for python 3.5 to anaconda and this new version clears up the problem in the Travis build.

While I was at it, I updated the conda build forslycot under python 3.3, which fixes up all of the errors on his PR.

@slivingstonslivingston merged commitd923c6b intopython-control:masterDec 26, 2016
slivingston added a commit that referenced this pull requestDec 26, 2016
@slivingston
Copy link
Member

Because it is not described in the message of commitca8f331, I note here that includingrcond instead ofinfo in the return-value ofhinfsyn is well motivated becausesb10ad raises an exception with theinfo value if it is nonzero, i.e., if theSB10AD subroutine did not have a successful exit. This behavior is documented in the respective parts of the API of Slycot and SLICOT:

  1. docstring ofsb10ad, in particular lines 1378-1413 of slycot/synthesis.py
  2. comments ofSB10AD, in particular lines 272-304 of slycot/src/SB10AD.f

@roryyorkeroryyorke deleted the rory-robust-test branchDecember 26, 2016 12:26
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers
No reviews
Assignees

@slivingstonslivingston

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

6 participants
@roryyorke@slivingston@murrayrm@cwrowley@coveralls@lyra25

[8]ページ先頭

©2009-2025 Movatter.jp