fixes for bug #46597 impulse response.

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

fixes for bug #46597 impulse response.

Doug Stewart-4
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:

Doug


--
DAS

Reply | Threaded
Open this post in threaded view
|

Re: fixes for bug #46597 impulse response.

Juan Pablo Carbajal-2
On Tue, Aug 30, 2016 at 4:41 PM, Doug Stewart <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: fixes for bug #46597 impulse response.

Doug Stewart-4


On Tue, Aug 30, 2016 at 11:43 AM, Juan Pablo Carbajal <[hidden email]> wrote:
On Tue, Aug 30, 2016 at 4:41 PM, Doug Stewart <[hidden email]> 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.


--
DASCertificate for 206392

Reply | Threaded
Open this post in threaded view
|

Re: fixes for bug #46597 impulse response.

Doug Stewart-4


On Aug 30, 2016 11:48 AM, "Doug Stewart" <[hidden email]> wrote:


On Tue, Aug 30, 2016 at 11:43 AM, Juan Pablo Carbajal <[hidden email]> wrote:
On Tue, Aug 30, 2016 at 4:41 PM, Doug Stewart <[hidden email]> 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


--
DASCertificate for 206392


Reply | Threaded
Open this post in threaded view
|

Re: fixes for bug #46597 impulse response.

Doug Stewart-4


On Wed, Jan 25, 2017 at 6:22 AM, Doug Stewart <[hidden email]> wrote:


On Aug 30, 2016 11:48 AM, "Doug Stewart" <[hidden email]> wrote:


On Tue, Aug 30, 2016 at 11:43 AM, Juan Pablo Carbajal <[hidden email]> wrote:
On Tue, Aug 30, 2016 at 4:41 PM, Doug Stewart <[hidden email]> 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
rezahousseini
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


Reply | Threaded
Open this post in threaded view
|

Re: fixes for bug #46597 impulse response.

Reza Housseini-2
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 <[hidden email]> wrote:


On Wed, Jan 25, 2017 at 6:22 AM, Doug Stewart <[hidden email]> wrote:


On Aug 30, 2016 11:48 AM, "Doug Stewart" <[hidden email]> wrote:


On Tue, Aug 30, 2016 at 11:43 AM, Juan Pablo Carbajal <[hidden email]> wrote:
On Tue, Aug 30, 2016 at 4:41 PM, Doug Stewart <[hidden email]> 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
rezahousseini
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



Reply | Threaded
Open this post in threaded view
|

Re: fixes for bug #46597 impulse response.

Doug Stewart-4


On Wed, Jan 25, 2017 at 9:51 AM, Reza Housseini <[hidden email]> wrote:
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

ping