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

Updated gram to support discrete-time systems#969

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
murrayrm merged 2 commits intopython-control:mainfrombasicmachines:main
Mar 30, 2024

Conversation

billtubbs
Copy link
Contributor

gram function for observability/controllability Gramians did not support discrete time systems, as described in#967 (comment).

Updatedcontrol/statefbk.py and tests incontrol/tests/statefbk_test.py.

@bnavigator
Copy link
Contributor

I have fixed the failing FutureWarning test. Could you please rebase onto current main?

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.

Thanks for working on these ancient TODO lines! They're 13 years old!

9b5b460

raise ValueError("Oops, the system is unstable!")

else:
raise ValueError("sys")
Copy link
Contributor

@bnavigatorbnavigatorFeb 18, 2024
edited
Loading

Choose a reason for hiding this comment

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

I imagine an error message from this line could be very confusing.

Is this even reachable by normal API usage? If so, could you please cover it in statefbk_test.py? If not, do we really need the line or is aif sys.isctime(): .. else: ... construct enough?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

This is true. The only time this would ever get raised is if someone passed an object that returns false to bothisctime andisdtime. I put it in because it's bad practice to doif cond1: ... elif cond2: with no else but it would only realistically get raised during development if something was wrong. That's why I left the message very simple because no real user is likely to ever see it, hopefully!

Copy link
ContributorAuthor

@billtubbsbilltubbsFeb 18, 2024
edited
Loading

Choose a reason for hiding this comment

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

Ifsys.isdtime literally returnednot sys.isctime then we can be sure this will never occur. But it looks more complicated than that.

Still, this might be a case of 'belt and braces' (belt and suspenders) and I'm happy to delete it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, this may be pedantic but for the sake of coverage, please create a test so that the test suite at least hits that line once.

@billtubbs
Copy link
ContributorAuthor

I have fixed the failing FutureWarning test. Could you please rebase onto current main?

Sorry, how do I do that again? I just tried usingthese instructions and not sure it worked.

@bnavigator
Copy link
Contributor

Sorry, how do I do that again? I just tried usingthese instructions and not sure it worked.

Those instructions are not complete for your case. Your PR comes from basicmachines/main. That makes it a bit more complicated than our own instructions inhttps://github.com/python-control/python-control/wiki/How-to-contribute-with-a-pull-request

You need to:

option A

  1. Sync your fork onhttps://github.com/basicmachines/python-control-2/tree/main using the sync fork button
  2. Assuming your local machine has above repository as remote "origin":
git fetch origingit rebase origin/maingit push -f

option B

# on branch main tracking basicmachines:main (https://github.com/basicmachines/python-control-2/tree/main)git remote add upstreamhttps://github.com/python-control/python-control.gitgit fetch upstreamgit rebase upstream/maingit push -f

(Why do you have two forks? Better work with properly named feature branches)

@billtubbs
Copy link
ContributorAuthor

(Why do you have two forks? Better work with properly named feature branches)

Sorry I probably shouldn't have created another fork. The previous fork has all that work I did on the plot functions previously (I shared a link to it with Richard in case he needs to refer to it). It's all far behind the origin now.

I'll fix that conditional statement so no test is needed and re-submit the PR.

@billtubbs
Copy link
ContributorAuthor

billtubbs commentedFeb 18, 2024
edited
Loading

I committed the change so it should be ready to merge now. I only ran the statefbk_test.py tests and there are still two fails but I don't think these are anything to do with the changes I made:

XFAIL control/tests/statefbk_test.py::TestStatefbk::testLQR_warning - warning not implementedXFAIL control/tests/statefbk_test.py::TestStatefbk::testDLQR_warning - warning not implemented

@coveralls
Copy link

coveralls commentedFeb 18, 2024
edited
Loading

Coverage Status

coverage: 94.425% (+0.003%) from 94.422%
when pulling0836ad8 on basicmachines:main
into7a70be1 on python-control:main.

@bnavigator
Copy link
Contributor

XFAIL

That is an "Expected fail" and nothing to worry about here.

@bnavigator
Copy link
Contributor

You still need to rebase to current upstream/main (e1e33e4).

The current diff (https://github.com/python-control/python-control/pull/969/files) and commit list (https://github.com/python-control/python-control/pull/969/commits) shows changes from Henrik's and my merged PRs which we do not want to duplicate.

git switch maingit remote -v | grep 'upstream.*python-control/python-control' || git remote add upstream https://github.com/python-control/python-control.gitgit fetch upstreamgit rebase upstream/maingit push -f

image

@sawyerbfuller
Copy link
Contributor

@billtubbs if you're getting tied up doing a rebase (as has often happened to me) maybe easier would be to try working on a fresh branch from an up-to-date main and re-doing your changes by hand?

@billtubbs
Copy link
ContributorAuthor

Hi Ben, Sawyer, sorry I got distracted and forgot to finish this. Thanks for the reminder I will get back to it this week.

@murrayrm
Copy link
Member

@billtubbs I'm likely to release a new version of python-control in the next day or two. If you have a chance to rebase this PR, I'll include it in the v0.10.0 (otherwise v0.10.1).

@billtubbs
Copy link
ContributorAuthor

Sorry for delay. I just rebased it using your earlier instructions and it seemed to work. Running the tests again now...

bnavigator reacted with thumbs up emoji

@billtubbs
Copy link
ContributorAuthor

Tests seemed to work. Is this success?

3550 passed, 30 skipped, 11 xfailed, 5 warnings in 133.34s (0:02:13)
bnavigator reacted with thumbs up emoji

@murrayrm
Copy link
Member

Thanks@billtubbs. The test output looks fine.

@murrayrmmurrayrm merged commit4d878a9 intopython-control:mainMar 30, 2024
@murrayrmmurrayrm added this to the0.10.0 milestoneMar 31, 2024
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@bnavigatorbnavigatorbnavigator left review comments

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

Successfully merging this pull request may close these issues.

5 participants
@billtubbs@bnavigator@coveralls@sawyerbfuller@murrayrm

[8]ページ先頭

©2009-2025 Movatter.jp