Re: fixes for bug #46597 impulse response.

[Reza Housseini]

Hi Doug

Sorry for taking so much time for looking at the code (though I didn't finish yet). I would strongly suggest merging this two impinvar to don't have duplicate code bases. But I agree that this can be done at a later stage but it should be noted somewhere in the bug tracker.

Also there is an example in the paper of Cavicchi which should be added to the tests.

I try to find some time to finish the review but bear with my delays.

Greetings

Reza

On Wed, Jan 25, 2017 at 3:35 PM, Doug Stewart <address@hidden> wrote:

On Wed, Jan 25, 2017 at 6:22 AM, Doug Stewart <address@hidden> wrote:

On Aug 30, 2016 11:48 AM, "Doug Stewart" <address@hidden> wrote:

On Tue, Aug 30, 2016 at 11:43 AM, Juan Pablo Carbajal <address@hidden> wrote:

On Tue, Aug 30, 2016 at 4:41 PM, Doug Stewart <address@hidden> wrote:
> I have mad a big change for the control pkg, to fix the impulse response.
> and would like someone to review it.
> Reza has been mentioned as someone that that would understand the work that
> I did.
> so Reza , do you have time to look it over?
>
> You can find it here:
> https://bitbucket.org/dougstew/
>
> Doug
>
>
> --
> DAS
>

I Doug, if you took my comment, I meant Reza Housseini

ok sorry to the wrong Reza.

Ping

No one has responded !!!

I have a major fix and it sits unused!!!

Disappointed  Doug

--

DAS

--

DAS

Thank you Reza for looking at this.

you asked

Reza Housseini commented on commit 3c5a0b8: added for impulse invariant transform for c2d Then why is this function redefined here when its already present in the signal package?

Some history:

When I started to debug the problem last winter, I was looking for a minor corner case that needed 

to be fixed. But as I went through the code I could not find and fix. Then I expanded my 

single stepping and found that the original coder had used the ZOH method to do the 

Impulse invariant method!!  Which is very wrong. At that point in time I did not know about impinvar

in the signal pkg. so I wrote my own imp_invar. I then did all the changes to make the control pkg use 

my inp_invar. And did may checks against matlab to make sure that it was the same as matlab.

At  about that time I became aware of impinvar in the signal pkg. and thought about using it. But

it's test call the control pkg to verify it's own answers. This would give a false sense if controls then

called signal's impinvar!!!. 

Also the signal pkg is dependent on the controls pkg, so I did not want to make the controls pkg 

dependent on the signals pkg  :-)

As it turns out my independent imp_invar give a slightly different result than than impinvar

from the  signals pkg and from Matlab. The difference is one time delay. My version follow a strict

math of:

   1) do the inverse Lapace transform

   2) covert from t (time ) space to kT (discreet time) space

   3) do the Z transform

 To then make it compatible one would remove a time delay ( a Z )

I did not do this in my inv_imvar because it was already handled in my other code.

(This could still be done)

The important thing right now is that we a shipping out control pkgs that are wrong,

and that there is a fix but there is no maintainer to apply the fix!!!!

Thank for looking at this and I will work with you or you can take what I have done

and change it any way you want. All I want is the control pkg to be fixed.

Doug