Skip to content
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

Closed
wants to merge 12 commits into from

Conversation

sawyerbfuller
Copy link
Contributor

@sawyerbfuller sawyerbfuller commented Jul 17, 2020

Changes the new default to be dt=0 (continuous-time), rather than dt=None (timebase unspecified) for state space, transfer function, and input output systems. This follows matlab convention as discussed in issue #422

  • new default is dt=0 in these three modules (edit: setting now is in a single location: defaults['control.default_dt'])
  • to make interconnection behavior match what is described in conventions, a new function: 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, eg dt=0.1).
  • new functions _isstaticgain (edit: now is a method is_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.
  • I needed to take dt out of the arguments list in 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.
  • fixes to unit tests
  • use_legacy_defaults('0.8.3') reverts to old default behavior of dt=None

@sawyerbfuller
Copy link
Contributor Author

sawyerbfuller commented Jul 17, 2020

Let me know what you think. If it looks good I will update documentation in modules and in conventions.rst

@coveralls
Copy link

coveralls commented Jul 17, 2020

Coverage Status

Coverage increased (+0.07%) to 84.278% when pulling dc53eb9 on sawyerbfuller:default_dt_0 into d3142ff on python-control:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 84.296% when pulling 9aed7cc on sawyerbfuller:default_dt_0 into d3142ff on python-control:master.

@sawyerbfuller
Copy link
Contributor Author

sawyerbfuller commented Jul 18, 2020

some remarks:

  • most of the unit tests worked as-is with the change, except for ones that implicitly assumed dt=None during the tests. After making that assumption explicit on such systems, pretty much everything worked
  • most of the effort came in implementing a test for static gain in StateSpace and TransferFunction classes (maybe one is needed in iosys?)
  • and in standardizing the use of lti.common_timebase everywhere in the software when interconnecting systems
  • I tested it on a few pages of my own code full of discrete and cont time systems and everything worked unchanged
  • all unit tests work for both dt=0 and dt=None as defaults

@murrayrm
Copy link
Member

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" default_dt with individual overrides if you want them.

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.

@sawyerbfuller
Copy link
Contributor Author

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.

Copy link
Contributor

@bnavigator bnavigator left a comment

Choose a reason for hiding this comment

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

I like it!

control/lti.py Show resolved Hide resolved
control/tests/discrete_test.py Outdated Show resolved Hide resolved
@sawyerbfuller
Copy link
Contributor Author

sawyerbfuller commented Jul 20, 2020

OK made a few suggested changes:

  • default dt is now package-wide. change it with set_defaults('control', default_dt=0)
  • re-incorporated timebaseEqual with deprecation warning
  • improved a few unit tests

.vscode/settings.json Outdated Show resolved Hide resolved
@sawyerbfuller
Copy link
Contributor Author

sawyerbfuller commented Jul 22, 2020

latest commit creates a new, potentially-useful method, is_static_gain() for StateSpace and TransferFunction systems. refactored their init methods to use it now rather than what it used to be, which was a function internal to the respective module

  • during refactor, added the ability to init using a dt keyword, as suggested in issue Problem with DT State-Space Response #419. If dt is received as both a positional and a keyword argument, the user receives a warning and the positional argument is used.

@sawyerbfuller
Copy link
Contributor Author

I think this should also resolve #399

@bnavigator
Copy link
Contributor

bnavigator commented Jul 23, 2020

Did someone check to merge your PRs with @samlaf's? They seem to work on overlapping functionality.

@sawyerbfuller
Copy link
Contributor Author

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.

@bnavigator
Copy link
Contributor

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.

@sawyerbfuller sawyerbfuller linked an issue Aug 19, 2020 that may be closed by this pull request
bnavigator added a commit to bnavigator/python-control that referenced this pull request Aug 20, 2020
bnavigator added a commit to bnavigator/python-control that referenced this pull request Aug 20, 2020
bnavigator added a commit to bnavigator/python-control that referenced this pull request Aug 20, 2020
revert this commit when merging into/with python-control#431

(remove statesp_test.py::test_copy_constructor_nodt if not applicable)
bnavigator added a commit to bnavigator/python-control that referenced this pull request Aug 20, 2020
bnavigator added a commit to bnavigator/python-control that referenced this pull request Aug 20, 2020
revert this commit when merging into/with python-control#431

(remove statesp_test.py::test_copy_constructor_nodt if not applicable)
bnavigator added a commit to bnavigator/python-control that referenced this pull request Dec 29, 2020
bnavigator added a commit to bnavigator/python-control that referenced this pull request Dec 29, 2020
bnavigator added a commit to bnavigator/python-control that referenced this pull request Dec 29, 2020
revert this commit when merging into/with python-control#431

(remove statesp_test.py::test_copy_constructor_nodt if not applicable)
bnavigator added a commit to bnavigator/python-control that referenced this pull request Dec 29, 2020
bnavigator added a commit to bnavigator/python-control that referenced this pull request Dec 29, 2020
bnavigator added a commit to bnavigator/python-control that referenced this pull request Dec 29, 2020
revert this commit when merging into/with python-control#431

(remove statesp_test.py::test_copy_constructor_nodt if not applicable)
bnavigator added a commit to bnavigator/python-control that referenced this pull request Dec 29, 2020
bnavigator added a commit to bnavigator/python-control that referenced this pull request Dec 29, 2020
bnavigator added a commit to bnavigator/python-control that referenced this pull request Dec 29, 2020
revert this commit when merging into/with python-control#431

(remove statesp_test.py::test_copy_constructor_nodt if not applicable)
bnavigator added a commit to bnavigator/python-control that referenced this pull request Dec 29, 2020
revert this commit when merging into/with python-control#431

(remove statesp_test.py::test_copy_constructor_nodt if not applicable)
bnavigator added a commit to bnavigator/python-control that referenced this pull request Dec 29, 2020
revert this commit when merging into/with python-control#431

(remove statesp_test.py::test_copy_constructor_nodt if not applicable)
bnavigator added a commit to bnavigator/python-control that referenced this pull request Dec 30, 2020
revert this commit when merging a rebased python-control#431

(remove statesp_test.py::test_copy_constructor_nodt if not applicable)
murrayrm pushed a commit that referenced this pull request Dec 30, 2020
* 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)
bnavigator added a commit to bnavigator/python-control that referenced this pull request Dec 31, 2020
@bnavigator
Copy link
Contributor

Superseded by #490

@bnavigator bnavigator closed this Dec 31, 2020
bnavigator added a commit that referenced this pull request Dec 31, 2020
@murrayrm murrayrm removed this from the 0.9.0 milestone Mar 20, 2021
@bnavigator bnavigator mentioned this pull request Mar 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants