-
Notifications
You must be signed in to change notification settings - Fork 438
Default dt is 0 #431
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
Default dt is 0 #431
Conversation
…re that combines systems with different timebases
Let me know what you think. If it looks good I will update documentation in modules and in conventions.rst |
some remarks:
|
aa6bf84
to
d59e57a
Compare
For the defaults, it seems like most people are going use the same default for transfer functions, state space systems, and I/O systems. Right now they have to set three defaults. Does it makes sense to collapse this to just one configuration variable? Or perhaps allow both: a "global" Also: since this will nominally break code, I propose putting this into v0.9 (versus 0.8.4, which we are close to releasing). Will try to take a closer look at the code itself in a bit. |
Hi Richard, this all makes sense and sounds reasonable. There was some reason (I think it was mostly expedience) for why I didn’t just use a a global default dt - I think I had decided that something about the global defaults hadn’t been completely implemented yet, but let me take a look, I agree that would be the right way to do it if possible. |
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 like it!
OK made a few suggested changes:
|
…s default system timebase
…tems, refactored __init__ on both to use it
latest commit creates a new, potentially-useful method,
|
I think this should also resolve #399 |
Did someone check to merge your PRs with @samlaf's? They seem to work on overlapping functionality. |
@bnavigator good to check. A brief glance through suggests that they are are basically operating on different aspects of the iosys functionality, and I don't anticipate any merge problems, except for the occasional empty line. The devil is in the details, of course. I don't know git will enough to do a test merge. If any issues crop up you can let me know and I will be happy to take a look at it. |
I made a test merge in bnavigator#1 and reactivated TravisCI for my fork. Tests look good so far, there were merge conflicts in both merges, because the edits were in lines next to each other. Please check, if I resolved correctly. Originally posted by @bnavigator in #433 (comment) I postet the comment in the wrong thread. Meant to post it here, so that @samlaf gets notified too. Alas, another mention. |
revert this commit when merging into or with python-control#400 and python-control#431
revert this commit when merging into/with python-control#431 (remove statesp_test.py::test_copy_constructor_nodt if not applicable)
revert this commit when merging into or with python-control#400 and python-control#431
revert this commit when merging into/with python-control#431 (remove statesp_test.py::test_copy_constructor_nodt if not applicable)
revert this commit when merging into or with python-control#400 and python-control#431
revert this commit when merging into/with python-control#431 (remove statesp_test.py::test_copy_constructor_nodt if not applicable)
revert this commit when merging into or with python-control#400 and python-control#431
revert this commit when merging into/with python-control#431 (remove statesp_test.py::test_copy_constructor_nodt if not applicable)
revert this commit when merging into or with python-control#400 and python-control#431
revert this commit when merging into/with python-control#431 (remove statesp_test.py::test_copy_constructor_nodt if not applicable)
revert this commit when merging into/with python-control#431 (remove statesp_test.py::test_copy_constructor_nodt if not applicable)
revert this commit when merging into/with python-control#431 (remove statesp_test.py::test_copy_constructor_nodt if not applicable)
revert this commit when merging a rebased python-control#431 (remove statesp_test.py::test_copy_constructor_nodt if not applicable)
* reorganize travis matrix, extend conftest.py * pytestify bdalg_test * pytestify canonical_test * pytestify config_test * pytestify convert_test.py * pytestify ctrlutil_test * pytestify delay_test.py * pytestify discrete_test * pytestify flatsys_test * pytestify frd_test * pytestify freqresp_test.py * pytestify input_element_int_test * pytestify iosys_test * pytestify lti_test.py * pytestify mateqn_test * pytestify matlab tests * pytestify minreal_test * pytestify modelsimp * pytestify nichols_test * pytestify phaseplot_test * pytestify rlocus_test.py * pytestify robust tests * pytestify sisotool_test.py * pytestify slycot_convert_test.py * pytestify statefbk tests * pytestify statesp tests * pytestify timeresp_test.py * pytestify xferfcn_input_test.py remove a lot of duplicate code by converting everything into a single parametrized test function. * pytestify xferfcn tests * make tests work with pre #431 source code state revert this commit when merging a rebased #431 (remove statesp_test.py::test_copy_constructor_nodt if not applicable)
…" from python-control#438 This reverts commit 2b98769.
Superseded by #490 |
Rebased #431: Default dt is 0
Changes the new default to be
dt=0
(continuous-time), rather thandt=None
(timebase unspecified) for state space, transfer function, and input output systems. This follows matlab convention as discussed in issue #422dt=0
in these three modules (edit: setting now is in a single location:defaults['control.default_dt']
)lti.common_timebase(dt1, dt2)
was introduced. It performs logic to correctly combine timebases of combined systems. The new function now correctly allows discrete-time systems with unspecified sampling time (dt=True
) to be combined with systems with specified sampling time (dt!=0
,dt!=None
, egdt=0.1
)._isstaticgain
(edit: now is a methodis_static_gain
) were added to detect upon initialization whether a state space or transfer function is a static gain. If so,dt=None
for such systems so they may be combined with either discrete- or continuous-time systems.iosys
init functions because library-wide defaults are not available when modules are first imported, which is when default values in function declarations are evaluated. My solution was to move the dt keyword in the**kwargs
dict.use_legacy_defaults('0.8.3')
reverts to old default behavior ofdt=None