Description

SeungKil-Paek

opened on Jun 20, 2020

Issue body actions

Dear all,

I found a bug in the connect function in the bdalg.py:

335: elif outp < 0 and -outp >= -sys.outputs:

if outp < 0, the index meant is -outp because the negative sign is to tell it is negative feedback.
(-outp) must be in the range [1, sys.outputs]. As outp < 0, -outp > 0 already. Hence, the remaining thing is that -outp <= sys.outputs. The code must be corrected like this:

335: elif outp < 0 and -outp <= sys.outputs:

Regards.

Seung-Kil

Activity

1. murrayrm commented on Jun 20, 2020

   

   murrayrm

   on Jun 20, 2020

   Member

   More actions

   If you can generate a simple example that illustrates the bug, this would help us create a unit test to go along with the fix.
2. SeungKil-Paek commented on Jun 21, 2020

   

   SeungKil-Paek

   on Jun 21, 2020

   Author

   More actions

   It looks to me that the intention of the current code is to neglect interconnections out of range .
   Hence, the result will be the same as that only with valid interconnections.

   I made test code like this:

   import numpy as np
   
   from control.statesp import StateSpace
   from control.bdalg import append, connect
   
   class Test(object):
       def __init__(self):
           self.sys1 = StateSpace([[1., -2], [3., -4]], [[5.], [7]], [[6, 8]], [[9.]])
           self.sys2 = StateSpace([[-1.]], [[1.]], [[1.]], [[0.]])
   
       def test_wrong_Q_arg(self):
           sys = append(self.sys1, self.sys2)
           
           # wrong negative feedback interconnection will be neglected
           Q1 = [[1, 2], [2, -3]] 
           ans1 = connect(sys, Q1, [2], [1, 2])
   
           Q2 = [[1, 2]]  
           ans2 = connect(sys, Q2, [2], [1, 2])
           np.testing.assert_array_almost_equal(ans1.A, ans2.A)
           np.testing.assert_array_almost_equal(ans1.B, ans2.B)
           np.testing.assert_array_almost_equal(ans1.C, ans2.C)
           np.testing.assert_array_almost_equal(ans1.D, ans2.D)
   
   Test().test_wrong_Q_arg()

   However, the test ends with IndexError.

   
   Volumes/ssd1/Applications/anaconda3/lib/python3.7/site-packages/control/bdalg.py in connect(sys, Q, inputv, outputv)
       334                 K[inp,outp-1] = 1.
       335             elif outp < 0 and -outp >= -sys.outputs:
   --> 336                 K[inp,-outp-1] = -1.
       337     sys = sys.feedback(np.array(K), sign=1)
       338 
   
   IndexError: index 2 is out of bounds for axis 1 with size 2
   

   I guess the code needs a range check of the variable inp also.
3. sawyerbfuller commented on Jun 22, 2020

   

   sawyerbfuller

   on Jun 22, 2020

   Contributor

   More actions

   I encountered a similar issue, but I'm not sure if it's the same:

   plant = control.ss(control.tf(1, [2, 1]))
   compensator = control.tf([10, 10], [100, 1])
   filter = control.tf(1, [10, 1])
   summer = control.ss([[0,0.],[0,0]], [[0,0.],[0,0]], [[0,0.],[0,0]], [[1,  0],[1,  1]])
   FFGain = control.tf(Kff,1)
   appended = control.append(plant, summer, FFGain, compensator, filter)
   connects = [
       [1,   +6,    0],
       [4,   -1,    0],
       [5,   +2,    0],
       [6,   +3,    0],
       [7,   +4,   +5]]
   inpts = [ 3, 2 ]   # [1] r input, [2] d (disturbance input)
   outpts = [ 1, ]  # [1] y (plant output)
   # bug/problem here: 
   closedloop = control.connect(appended, connects, inpts, outpts)
   

   gives

   ~/Documents/src/python-control-dev/python-control/control/bdalg.py in connect(sys, Q, inputv, outputv)
       332         for outp in r[1:]:
       333             if outp > 0 and outp <= sys.outputs:
   --> 334                 K[inp,outp-1] = 1.
       335             elif outp < 0 and -outp >= -sys.outputs:
       336                 K[inp,-outp-1] = -1.
   
   IndexError: index 6 is out of bounds for axis 0 with size 6
   
4. murrayrm

   self-assigned this

   on Jun 28, 2020
5. murrayrm

   added

   bug

   on Jun 28, 2020
6. murrayrm

   added this to the 0.8.4 milestone on Jul 11, 2020
7. roryyorke commented on Aug 22, 2020

   

   roryyorke

   on Aug 22, 2020

   Contributor

   More actions

   Do we want to ignore out-of-range input and output indices here, or raise an error? I'd lean towards reporting an error. This applies to Q, inputv, and outputv.
8. murrayrm commented on Aug 22, 2020

   

   murrayrm

   on Aug 22, 2020

   Member

   More actions

   Agree, it should raise an error. I'm bogged down in a bunch of other things right now, but hope to have some time first week of September. If someone else wants to take a look between now and then, that would be great.
9. sawyerbfuller commented on Aug 22, 2020

   

   sawyerbfuller

   on Aug 22, 2020

   Contributor

   More actions

   I second Richard’s sentiment - should raise an error.
10. sawyerbfuller

    added a commit that references this issue on Dec 19, 2020

    bdalg.connect: added and fixed index checks to fix python-control#421;…

    This issue will close once commit 85cc6ca is merged into the 'master' branch.

    85cc6ca
11. sawyerbfuller commented on Dec 19, 2020

    

    sawyerbfuller

    on Dec 19, 2020

    Contributor

    More actions

    Ok I think I fixed it in #474. The error that I found above that I thought was related was actually in other code.
12. sawyerbfuller

    linked a pull request that will close this issueFix bdalg.connect #474on Dec 19, 2020
13. bnavigator

    closed this as completedin #474on Dec 21, 2020

Issue actions