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

Fix bdalg.connect#474

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
bnavigator merged 3 commits intopython-control:masterfromsawyerbfuller:fix-connect
Dec 21, 2020

Conversation

sawyerbfuller
Copy link
Contributor

Fixes bug in index check inbdalg.connect described in#421.

Also added a few more checks, helpful error messages, and unit tests.

@sawyerbfullersawyerbfuller changed the titleFix connectFix bdalg.connectDec 19, 2020
@coveralls
Copy link

coveralls commentedDec 19, 2020•
edited
Loading

Coverage Status

Coverage increased (+0.04%) to 86.187% when pulling9c5b5e4 on sawyerbfuller:fix-connect into2d7aad0 on python-control:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 86.052% when pulling85cc6ca on sawyerbfuller:fix-connect into2d7aad0 on python-control:master.

@sawyerbfullersawyerbfuller linked an issueDec 19, 2020 that may beclosed by this pull request
@roryyorke
Copy link
Contributor

Looks good to me. One minor stylistic comment: you don't needas context if you're not going to usecontext.

sawyerbfuller reacted with thumbs up emoji

Copy link
Contributor

@bnavigatorbnavigator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Looks good, thanks! One step closer to 0.8.4 🎉

Some more comments about style and the tests.

@@ -270,6 +272,46 @@ def test_feedback_args(self):
sys = ctrl.feedback(1, frd)
self.assertTrue(isinstance(sys, ctrl.FRD))

def testConnect(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

These tests only look for failures. What about tests that successfully connect systems?

Obviously I will pytest.parametrize this for#438

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

These tests only look for failures. What about tests that successfully connect systems?

good idea; added.

Obviously I will pytest.parametrize this for#438

thx!

@bnavigatorbnavigator added this to the0.8.4 milestoneDec 19, 2020
@bnavigatorbnavigator merged commitde3509e intopython-control:masterDec 21, 2020
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@bnavigatorbnavigatorbnavigator approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.8.4
Development

Successfully merging this pull request may close these issues.

A bug in bdalg.connect function
4 participants
@sawyerbfuller@coveralls@roryyorke@bnavigator
[8]ページ先頭
©2009-2025 Movatter.jp