Skip to content

Fix or silence ruff check warnings; add ruff check to Github Actions #246

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Feb 1, 2025
Merged

Fix or silence ruff check warnings; add ruff check to Github Actions #246

merged 3 commits into from
Feb 1, 2025

Conversation

roryyorke
Copy link
Collaborator

Is this worth it at all?

Are the particular fixes I've made (especially # noqa ones) reasonable?

@roryyorke
Run "ruff check" on source in Github action
f305cc5
@roryyorke
Make source ruff check-clean
8978584 
Removed unused imports.

Removed unused variables in test: these were cut-and-paste from other
tests, and not needed for the tests in questions.

Other minor fixes.
@roryyorke roryyorke requested a review from bnavigator January 19, 2025 11:27
@roryyorke roryyorke mentioned this pull request Jan 19, 2025
Open
@bnavigator
Copy link
Collaborator

I like the general approach! Thanks @roryyorke!

I would prefer not to override E741 (ambigous variable name) on single lines. I'd rather just switch it off globally. We use the single letter variable names where SLICOT uses them.

bnavigator
bnavigator requested changes Jan 30, 2025
View reviewed changes
slycot/analysis.py Outdated
@@ -1838,7 +1838,7 @@ def ab13md(Z, nblock, itype, x=None):
return bound, d, g, x[:m+mr-1]


def ag08bd(l,n,m,p,A,E,B,C,D,equil='N',tol=0.0,ldwork=None):
def ag08bd(l,n,m,p,A,E,B,C,D,equil='N',tol=0.0,ldwork=None): # noqa: E741
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just switch off E741 in a global option in pyproject.toml

[tool.ruff.lint]
ignore = [ "E741" ]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done. If it weren't backward incompatible I'd be tempted to change l to ell, but so it goes.

@roryyorke
Copy link
Collaborator Author

From what I see, all failures are on macos, and I think all macos jobs fail. I checked two at random, and the failure is in python-control TestStateSpace.test_pow:

___________________________ TestStateSpace.test_pow ____________________________
  
  self = <control.tests.statesp_test.TestStateSpace object at 0x10b791ff0>
  sys222 = StateSpace(
  array([[ 4.,  1.],
         [ 2., -3.]]),
  array([[ 5.,  2.],
         [-3., -3.]]),
  array([[ 2., -4.],
         [ 0.,  1.]]),
  array([[ 3.,  2.],
         [ 1., -1.]]),
  states=2, outputs=2, inputs=2)
  sys322 = StateSpace(
  array([[-3.,  4.,  2.],
         [-1., -3.,  0.],
         [ 2.,  5.,  3.]]),
  array([[ 1.,  4.],
         [-3., ..., -3.],
         [ 1.,  4.,  3.]]),
  array([[-2.,  4.],
         [ 0.,  1.]]),
  name='sys322', states=3, outputs=2, inputs=2)
  
      @slycotonly
      def test_pow(self, sys222, sys322):
          """Test state space powers."""
          for sys in [sys222, sys322]:
              # Power of 0
              result = sys**0
              expected = StateSpace([], [], [], np.eye(2), dt=0)
              np.testing.assert_allclose(expected.A, result.A)
              np.testing.assert_allclose(expected.B, result.B)
              np.testing.assert_allclose(expected.C, result.C)
              np.testing.assert_allclose(expected.D, result.D)
              # Power of 1
              result = sys**1
              expected = sys
              np.testing.assert_allclose(expected.A, result.A)
              np.testing.assert_allclose(expected.B, result.B)
              np.testing.assert_allclose(expected.C, result.C)
              np.testing.assert_allclose(expected.D, result.D)
              # Power of -1 (inverse of biproper system)
              # Testing transfer function representations to avoid the
              # non-uniqueness of the state-space representation. Once MIMO
              # canonical forms are supported, can check canonical state-space
              # matrices instead.
              result = (sys * sys**-1).minreal()
              expected = StateSpace([], [], [], np.eye(2), dt=0)
              assert _tf_close_coeff(
                  ss2tf(expected).minreal(),
                  ss2tf(result).minreal(),
              )
              result = (sys**-1 * sys).minreal()
              expected = StateSpace([], [], [], np.eye(2), dt=0)
  >           assert _tf_close_coeff(
                  ss2tf(expected).minreal(),
                  ss2tf(result).minreal(),
              )
  E           assert False
  E            +  where False = _tf_close_coeff(TransferFunction(\n[[array([1.]), array([0.])],\n [array([0.]), array([1.])]],\n[[array([1.]), array([1.])],\n [array([1.]), array([1.])]],\noutputs=2, inputs=2), TransferFunction(\n[[array([1.]), array([-2.76395297e-15, -1.35541257e-13,  9.01576541e-13])],\n [array([0.]), array([1.])]],\n[[array([1.]), array([  1.,   3.,  -9., -41.])],\n [array([1.]), array([1.])]],\noutputs=2, inputs=2))
  E            +    where TransferFunction(\n[[array([1.]), array([0.])],\n [array([0.]), array([1.])]],\n[[array([1.]), array([1.])],\n [array([1.]), array([1.])]],\noutputs=2, inputs=2) = minreal()
  E            +      where minreal = TransferFunction(\n[[array([1.]), array([0.])],\n [array([0.]), array([1.])]],\n[[array([1.]), array([1.])],\n [array([1.]), array([1.])]],\noutputs=2, inputs=2).minreal
  E            +        where TransferFunction(\n[[array([1.]), array([0.])],\n [array([0.]), array([1.])]],\n[[array([1.]), array([1.])],\n [array([1.]), array([1.])]],\noutputs=2, inputs=2) = ss2tf(StateSpace(\narray([], shape=(0, 0), dtype=float64),\narray([], shape=(0, 2), dtype=float64),\narray([], shape=(2, 0), dtype=float64),\narray([[1., 0.],\n       [0., 1.]]),\nstates=0, outputs=2, inputs=2))
  E            +    and   TransferFunction(\n[[array([1.]), array([-2.76395297e-15, -1.35541257e-13,  9.01576541e-13])],\n [array([0.]), array([1.])]],\n[[array([1.]), array([  1.,   3.,  -9., -41.])],\n [array([1.]), array([1.])]],\noutputs=2, inputs=2) = minreal()
  E            +      where minreal = TransferFunction(\n[[array([  1.,   3.,  -9., -41.]), \n  array([-2.76395297e-15, -1.35541257e-13,  9.01576541e-13])],\n ....,   3.,  -9., -41.]), array([  1.,   3.,  -9., -41.])],\n [array([1.]), array([1., 0., 0., 0.])]],\noutputs=2, inputs=2).minreal
  E            +        where TransferFunction(\n[[array([  1.,   3.,  -9., -41.]), \n  array([-2.76395297e-15, -1.35541257e-13,  9.01576541e-13])],\n ....,   3.,  -9., -41.]), array([  1.,   3.,  -9., -41.])],\n [array([1.]), array([1., 0., 0., 0.])]],\noutputs=2, inputs=2) = ss2tf(StateSpace(\narray([[  4.86563613,  -0.22114886, -14.38268526],\n       [  3.86388385,  -1.64824725, -16.2652334 ],\n    ... [0.00000000e+00, 2.59786829e-16, 3.85014366e-15]]),\narray([[1., 0.],\n       [0., 1.]]),\nstates=3, outputs=2, inputs=2))
  
  control/tests/statesp_test.py:596: AssertionError

@roryyorke
Copy link
Collaborator Author

not all macos tests fail; many pass, but 7 fail.

@bnavigator
Copy link
Collaborator

Failing macOS tests are unrelated. Thanks @roryyorke

@bnavigator bnavigator merged commit bbfe718 into python-control:master Feb 1, 2025
64 of 71 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnavigator bnavigator bnavigator requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@roryyorke @bnavigator