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 — Lint tests by roryyorke · Pull Request #1127 · python-control/python-control

Conversation

@roryyorke

@roryyorke roryyorke commented Feb 16, 2025

Copy link
Copy Markdown
Contributor

This extends ruff testing to control/tests/*.py

I've tried to order the commits from obvious to subtle; on the whole:

  • remove unused imports
  • removed unused variables, or sometimes prefix with '_' where that seemed to make things clearer
  • tweaked two eval calls

The remaining errors I'm less sure of. It would be simple to silence them with # noqa: F<relevant code>, and defer fixing these to later; then at least no new ruff errors will sneak in in the future.

More detail on remaining failures:

control/tests/kwargs_test.py

Here the dict literal has repeated keys for 'StateSpace.__init__' and 'StateSpace.__sample':

    'StateSpace.__init__':
        interconnect_test.test_interconnect_exceptions,
    'StateSpace.sample': test_unrecognized_kwargs,
    'NonlinearIOSystem.__init__':
        interconnect_test.test_interconnect_exceptions,
    'StateSpace.__init__': test_unrecognized_kwargs,
    'StateSpace.initial_response': timeresp_test.test_timeresp_aliases,
    'StateSpace.sample': test_unrecognized_kwargs,

Last-value-wins in dict literals, so the first two entries have no effect. It looks like this is just a check that something exists, so I guess we can just erase the first two entries?

control/tests/modelsimp_test.py

The variable mrk is assigned to: mrk = markov(outp, inp, 1, transpose=False). I didn't investigate issue #395 referenced above -- is it enough that this call doesn't raise?

control/tests/optimal_test.py

curved_seg_length is calculated, but not used. It's not obviously copy-and-pasted from another test, so I'm not sure if the test should be changed here. It's obviously easy enough to just remove this variable, and straight_seg_length just above it.

Sorry, something went wrong.

Uh oh!

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

All reactions
roryyorke added 17 commits February 16, 2025 10:31
@roryyorke
Lint: remove unused variables, or prefix with _
d5156e5
Where it's clear that function is called for side effects (e.g., in a
q`with pytest.raises` block), don't assign function output.  Where
it's not clear, e.g., binary ops on LTI objects, call result `_sys` or
similar.  There are plenty of in-between cases: for those I chose
based on understandability.
@roryyorke
Remove unused imports
2a18331
Where imports were test fixtures, replaced with
`@pytest.mark.usefixtures('nameoffixture')`.
@roryyorke
Remove imports needed for eval
1bc0119
Provide relevant symbols via `locals` argument to eval.
@roryyorke
Fix incorrect variable names in f-strings
df756eb
@roryyorke
Apply ruff checks to control/tests/
48d6195
@roryyorke
Remove unused variable fail_if_missing in test_parameter_docs
20f4b76
@roryyorke
Remove unused obj and objname in test_iosys_attribute_lists
3729d89
@roryyorke
Remove unused variable docstring in test_iosys_container_classes
36d5e8a
@roryyorke
Import numpy for symbol np.array
7fe9d4e
Symbol reference in lambda which is never called.
@roryyorke
Remove ineffective testFeedback2 in frd_test.py
ba33c21
The test had no assertions, but have been intended to test MIMO
feedback; for this see testMIMOfb in same file.
@roryyorke
Add assertion in testUnwrap in matlab_test
2156adf
@roryyorke
Remove unused steady-state output variable
69afb82
@roryyorke
Mark unused test function with noqa
4e0b9c1
The function doesn't exist, but the test is never called.
@roryyorke
Merge remote-tracking branch 'upstream/main' into lint-tests
3654163
@roryyorke
Fix bugs and lint errors in test_timeresp_aliases
2d5738a
@roryyorke
Correct use of slycotonly in statesp_test.py
fb0519c
slycontonly is not a fixture.
@roryyorke
Remove unused ss_{siso,mimo} variables
c6d26f1
It looks like a block of code was copied-and-pasted between two test
functions; removed the unused variable in each case.
@roryyorke
roryyorke marked this pull request as draft February 16, 2025 10:42
@roryyorke
Don't use keyword args for eval
22db2bf
Only works from Python 3.13.
@coveralls

coveralls commented Feb 16, 2025 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown

Coverage Status

coverage: 94.744%. remained the same
when pulling c907a4f on roryyorke:lint-tests
into 2a919cc on python-control:main.

All reactions

Sorry, something went wrong.

Uh oh!

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

@roryyorke
Handle deprecation warnings from conda setup in Github actions
5ff3c0c
@murrayrm

murrayrm commented Feb 16, 2025

Copy link
Copy Markdown
Member

control/tests/kwargs_test.py

Correct that there just needs to be some test that verifies that unknown keywords generate an error. Whichever of the tests that has that check should be kept and if both do then keeping either is fine.

control/tests/modelsimp_test.py

Correct that the intent of this test was just to make sure an exception wasn't raised. We could either get rid of the assignment to mrk or check the output to make sure it does something sensible. For example:

np.testing.assert_almost_equal(mrk, 2.)

control/tests/optimal_test.py

I'm pretty sure I wrote this example and I can't see any reason to keep straight_seg_length or curved_seg_length.

All reactions

Sorry, something went wrong.

Uh oh!

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

roryyorke added 4 commits February 16, 2025 19:39
@roryyorke
Remove redundant kwarg test entries
51eb00a
@roryyorke
Add assertion to check result of markov call
e29ba64
@roryyorke
Remove unused variables in test_optimal_doc
ff4d7b6
@roryyorke
Merge remote-tracking branch 'upstream/main' into lint-tests
870fa19
@roryyorke
roryyorke marked this pull request as ready for review February 16, 2025 17:45
@roryyorke roryyorke changed the title [WIP] Lint tests Lint tests Feb 16, 2025
@roryyorke
Revert "Handle deprecation warnings from conda setup in Github actions"
c907a4f
This reverts commit 5ff3c0c.
@roryyorke

roryyorke commented Feb 16, 2025

Copy link
Copy Markdown
Contributor Author

Oops - had a commit there I'd intended for another PR. Reverted.

If this PR isn't merged now, it'll be worth merging main into the PR before merging the PR to main, to make sure the lint checks don't causes failures on main after the merge of the PR. (I hope that sentence is less confusing than I think it is!)

All reactions

Sorry, something went wrong.

Uh oh!

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

@murrayrm
murrayrm merged commit f6799ab into python-control:main Feb 19, 2025
@murrayrm murrayrm added this to the 0.10.2 milestone Feb 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

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

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.10.2

Development

Successfully merging this pull request may close these issues.

Uh oh!

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

3 participants

@roryyorke @coveralls @murrayrm
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.