GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Switch LTI class and subclasses to use ninputs, noutputs, nstates by murrayrm · Pull Request #515 · python-control/python-control

Conversation

@murrayrm

@murrayrm murrayrm commented Jan 20, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

This PR changes the LTI class and the StateSpace class (and all derived classes) to use ninputs, noutputs, and nstates as the attributes for storing these properties instead of inputs, outputs, and states. The former names are consistent with what is used in the InputOutputSystem class.

Following the suggestion of @bnavigator in issue #451, getter and setter functions are used for backward compatibility, but with a PendingDeprecationWarning.

Note that this PR will generate some small conflicts with #514. I'll rebase based on which one we merge first. [done]

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@murrayrm murrayrm added this to the 0.9.0 milestone Jan 20, 2021
@murrayrm murrayrm mentioned this pull request Jan 20, 2021
Release 0.8.4 and 0.9.0 sequencing #451
Closed
@coveralls

coveralls commented Jan 20, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.06%) to 87.553% when pulling eb401a9 on murrayrm:input_output_to_ninput_noutput into 1502d38 on python-control:master.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sawyerbfuller

sawyerbfuller commented Jan 20, 2021

Copy link
Copy Markdown
Contributor

Awesome! Looks good to me.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sawyerbfuller

sawyerbfuller commented Jan 20, 2021

Copy link
Copy Markdown
Contributor

One more thing I noticed: there are a few mentions of sys.inputs/outputs in docs in xferfunc.py that should probably be fixed.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@murrayrm
convert inputs, outputs, ... to ninputs... w/ getter/setter warning
5a4c3ab
@murrayrm
murrayrm force-pushed the input_output_to_ninput_noutput branch from 374c82f to 5a4c3ab Compare January 21, 2021 00:53
@murrayrm

murrayrm commented Jan 21, 2021

Copy link
Copy Markdown
Member Author

Rebased against master (and apparently fixed up the xferfcn.py docstring, since I can't find sys.inputs in that file anymore).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sawyerbfuller

sawyerbfuller commented Jan 21, 2021 via email •
edited by murrayrm
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor
May also need to check for self.inputs ?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@murrayrm
update getter/setter comments to match code
9d469f1
sawyerbfuller
sawyerbfuller reviewed Jan 23, 2021
View reviewed changes

@sawyerbfuller sawyerbfuller left a comment

Copy link
Copy Markdown
Contributor

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.

One more comment: was it a deliberate choice to leave init keywords as inputs/outputs rather than ninputs/noutputs, eg in lti.py:

def init(self, inputs=1, outputs=1, dt=None):

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@murrayrm
add unit test for input/output/state deprecation warning
f633874
@murrayrm
murrayrm force-pushed the input_output_to_ninput_noutput branch from d614c01 to f633874 Compare January 23, 2021 18:37
@murrayrm

murrayrm commented Jan 24, 2021

Copy link
Copy Markdown
Member Author

was it a deliberate choice to leave init keywords as inputs/outputs rather than ninputs/noutputs,

This matches the syntax we use in the InputOutputSystem class constructor. In that setting (and perhaps later here as well), inputs and outputs can be an integer or a list of strings (which get used for signal names).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sawyerbfuller

sawyerbfuller commented Jan 24, 2021

Copy link
Copy Markdown
Contributor

That makes sense. Thanks!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bnavigator
bnavigator reviewed Jan 24, 2021
View reviewed changes
Comment thread control/lti.py Outdated
Comment on lines +64 to +67
raise PendingDeprecationWarning(
"The LTI `inputs` attribute will be deprecated in a future "
"release. Use `ninputs` instead.")
return self.ninputs

@bnavigator bnavigator Jan 24, 2021

Copy link
Copy Markdown
Contributor

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.

This raises an Exception of the class PendingDeprecationWarning and never returns self.inputs.

Python 3.8.6 (default, Nov 09 2020, 12:09:06) [GCC] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import control
>>> rsys = control.rss(3,3,3)
>>> ninputs = rsys.inputs
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/ben/src/python-control/control/lti.py", line 64, in inputs
    raise PendingDeprecationWarning(
PendingDeprecationWarning: The LTI `inputs` attribute will be deprecated in a future release.  Use `ninputs` instead.
>>> ninputs
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
NameError: name 'ninputs' is not defined
Suggested change
raise PendingDeprecationWarning(
"The LTI `inputs` attribute will be deprecated in a future "
"release. Use `ninputs` instead.")
return self.ninputs
warn("The LTI `inputs` attribute will be deprecated in a future "
"release. Use `ninputs` instead.",
PendingDeprecationWarning, stacklevel=2)
return self.ninputs

Additionally, either a warning filter needs to be set in order to make the warning visible to end users, or a different category than *DeprecationWarning needs to be used.

filterwarnings('module',
               message=".*LTI.*attribute.*deprecated",
               category=PendingDeprecationWarning)
Python 3.8.6 (default, Nov 09 2020, 12:09:06) [GCC] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import control; rsys = control.rss(3,3,3); ninputs = rsys.inputs
<stdin>:1: PendingDeprecationWarning: The LTI `inputs` attribute will be deprecated in a future release.  Use `ninputs` instead.
>>> ninputs
3
  • Please add a check using pytest.warns().

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@murrayrm murrayrm Jan 24, 2021

Copy link
Copy Markdown
Member Author

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've fixed these up and decide to use DeprecationWarning for now (keep things internal to developers) but in a future release (0.9.x) we should change to FutureWarning, which will make it show up for users as well.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@bnavigator bnavigator Jan 24, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.

Still using raise instead of warn. Now we are at:

This raises an Exception of the class DeprecationWarning and never returns self.inputs.

Python 3.8.6 (default, Nov 09 2020, 12:09:06) [GCC] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import control; rsys = control.rss(3,3,3); ninputs = rsys.inputs
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/ben/src/python-control/control/lti.py", line 64, in inputs
    raise DeprecationWarning(
DeprecationWarning: The LTI `inputs` attribute will be deprecated in a future release.  Use `ninputs` instead.
>>> ninputs
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
NameError: name 'ninputs' is not defined
>>>

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
bnavigator
bnavigator reviewed Jan 24, 2021
View reviewed changes
Comment thread control/tests/lti_test.py Outdated
Comment on lines +254 to +255
with pytest.raises(DeprecationWarning, match="LTI `inputs`"):
assert sys.inputs == sys.ninputs

@bnavigator bnavigator Jan 24, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.

The assert .. == part is never evaluated after sys.ninputs throws the exception.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@murrayrm murrayrm Jan 24, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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.

Sorry, was working on multiple changes/branches at once and pushed the wrong version. New one was what was intended.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@murrayrm
murrayrm force-pushed the input_output_to_ninput_noutput branch 2 times, most recently from bed8990 to ff5fb39 Compare January 24, 2021 18:08
@murrayrm
fix up deprecation warnings in response to @bnavigator review
eb401a9
@murrayrm
murrayrm force-pushed the input_output_to_ninput_noutput branch from ff5fb39 to eb401a9 Compare January 24, 2021 18:10
bnavigator
bnavigator approved these changes Jan 24, 2021
View reviewed changes
@bnavigator
bnavigator merged commit eb146a6 into python-control:master Jan 24, 2021
@murrayrm
murrayrm deleted the input_output_to_ninput_noutput branch January 31, 2021 00:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sawyerbfuller sawyerbfuller sawyerbfuller left review comments
@bnavigator bnavigator bnavigator approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.9.0

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@murrayrm @coveralls @sawyerbfuller @bnavigator
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.