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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed

Conversation

sawyerbfuller
Copy link
Contributor

@sawyerbfullersawyerbfuller commentedJul 17, 2020
edited
Loading

Changes the new default to bedt=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#422

  • new default isdt=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 inconventions, 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, egdt=0.1).
  • new functions_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.
  • I needed to take dt out of the arguments list iniosys 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 ofdt=None

@sawyerbfuller
Copy link
ContributorAuthor

sawyerbfuller commentedJul 17, 2020
edited
Loading

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

@coveralls
Copy link

coveralls commentedJul 17, 2020
edited
Loading

Coverage Status

Coverage increased (+0.07%) to 84.278% when pullingdc53eb9 on sawyerbfuller:default_dt_0 intod3142ff on python-control:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 84.296% when pulling9aed7cc on sawyerbfuller:default_dt_0 intod3142ff on python-control:master.

@sawyerbfuller
Copy link
ContributorAuthor

sawyerbfuller commentedJul 18, 2020
edited
Loading

some remarks:

  • most of the unit tests worked as-is with the change, except for ones that implicitly assumeddt=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 inStateSpace andTransferFunction classes (maybe one is needed iniosys?)
  • and in standardizing the use oflti.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
ContributorAuthor

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

@bnavigatorbnavigator 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!

@sawyerbfuller
Copy link
ContributorAuthor

sawyerbfuller commentedJul 20, 2020
edited
Loading

OK made a few suggested changes:

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

@sawyerbfuller
Copy link
ContributorAuthor

sawyerbfuller commentedJul 22, 2020
edited
Loading

latest commit creates a new, potentially-useful method,is_static_gain() for StateSpace and TransferFunction systems. refactored theirinit 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 toinit using a dt keyword, as suggested in issueProblem 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
ContributorAuthor

I think this should also resolve#399

@bnavigator
Copy link
Contributor

bnavigator commentedJul 23, 2020
edited
Loading

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

@sawyerbfuller
Copy link
ContributorAuthor

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 inbnavigator#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.

@sawyerbfullersawyerbfuller linked an issueAug 19, 2020 that may beclosed by this pull request
bnavigator added a commit to bnavigator/python-control that referenced this pull requestAug 20, 2020
bnavigator added a commit to bnavigator/python-control that referenced this pull requestAug 20, 2020
bnavigator added a commit to bnavigator/python-control that referenced this pull requestAug 20, 2020
revert this commit when merging into/withpython-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 requestAug 20, 2020
bnavigator added a commit to bnavigator/python-control that referenced this pull requestAug 20, 2020
revert this commit when merging into/withpython-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 requestDec 29, 2020
bnavigator added a commit to bnavigator/python-control that referenced this pull requestDec 29, 2020
bnavigator added a commit to bnavigator/python-control that referenced this pull requestDec 29, 2020
revert this commit when merging into/withpython-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 requestDec 29, 2020
bnavigator added a commit to bnavigator/python-control that referenced this pull requestDec 29, 2020
bnavigator added a commit to bnavigator/python-control that referenced this pull requestDec 29, 2020
revert this commit when merging into/withpython-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 requestDec 29, 2020
bnavigator added a commit to bnavigator/python-control that referenced this pull requestDec 29, 2020
bnavigator added a commit to bnavigator/python-control that referenced this pull requestDec 29, 2020
revert this commit when merging into/withpython-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 requestDec 29, 2020
revert this commit when merging into/withpython-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 requestDec 29, 2020
revert this commit when merging into/withpython-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 requestDec 30, 2020
revert this commit when merging a rebasedpython-control#431(remove statesp_test.py::test_copy_constructor_nodt if not applicable)
murrayrm pushed a commit that referenced this pull requestDec 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.pyremove a lot of duplicate code by converting everything intoa single parametrized test function.* pytestify xferfcn tests* make tests work with pre#431 source code staterevert 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 requestDec 31, 2020
@bnavigator
Copy link
Contributor

Superseded by#490

bnavigator added a commit that referenced this pull requestDec 31, 2020
@murrayrmmurrayrm removed this from the0.9.0 milestoneMar 20, 2021
@bnavigatorbnavigator mentioned this pull requestMar 29, 2021
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
No milestone
4 participants
@sawyerbfuller@coveralls@murrayrm@bnavigator

[8]ページ先頭

©2009-2025 Movatter.jp