- Notifications
You must be signed in to change notification settings - Fork441
Rebased #431: Default dt is 0#490
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…" frompython-control#438This reverts commit2b98769.
…re that combines systems with different timebases
…s default system timebase
…tems, refactored __init__ on both to use it
coveralls commentedDec 31, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
coveralls commentedDec 31, 2020
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.
I look through the changes and these look good to me.
| @@ -260,8 +258,6 @@ def testAddition(self, tsys): | |||
| TransferFunction.__add__(tsys.siso_tf1c, tsys.siso_tf1d) | |||
| with pytest.raises(ValueError): | |||
| TransferFunction.__add__(tsys.siso_tf1d, tsys.siso_tf2d) | |||
| with pytest.raises(ValueError): | |||
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.
I assume these are removed because these tests are already performed above in eg ‘test_timenase_conversion’?
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.
They don't raise an error anymore. You had an equivalent change in#431.dt=0.1 anddt=True are compatible now.
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.
I see, makes sense!
| @@ -223,7 +223,7 @@ def __init__(self, *args, **kwargs): | |||
| # TODO: not sure this can ever happen since dt is always present | |||
| try: | |||
| dt = args[0].dt | |||
| exceptNameError: # pragma: no coverage | |||
| exceptAttributeError: | |||
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.
🤙
sawyerbfuller left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Looks good to me!
Added one more simple test in order to not uncover a line. The double dt resolver is documented behavior and thus should be checked. |
Rebased version of#431. I had the updated tests already prepared in earlier versions of#438