mhuser / scikit-rf

RF and Microwave Engineering Scikit
http://scikit-rf.org
BSD 3-Clause "New" or "Revised" License
0 stars 0 forks source link

Possible error in ZC Classes due to Network.flipped() #3

Closed denzchoe closed 2 years ago

denzchoe commented 2 years ago

Hi @mhuser,


I trace the code between TG1 and skrf. Perhaps there might be a small mistake here?

Specifically the Network flipping over here in python.

https://github.com/scikit-rf/scikit-rf/blob/97a3428cf9bafadb7f8f2c24af022b116ac94bbf/skrf/calibration/deembedding.py#L1823

    errorbox1 = errorbox1 ** sTL1
    errorbox2 = errorbox2 ** sTL2
 s_dut = sTL1.inv ** s_dut ** sTL2.flipped().inv  # Eq__(1)

and in MATLAB (no flipping done on TL2)

https://opensource.ieee.org/elec-char/ieee-370/-/blob/master/TG1/IEEEP370Zc2xThru.m#L486

        errorbox1 = cascadesparams(errorbox1,TL1);
        errorbox2 = cascadesparams(errorbox2,TL2);
    end

    s_dut = removeTL(s_dut,TL1,TL2,z0);

function out = removeTL(in,TL1,TL2,z0)
.
.
.
    for j = 1:n
        abcd_in(:,:,j) = abcd_TL1(:,:,j)\abcd_in(:,:,j)/abcd_TL2(:,:,j);   % Eq__(2) denz: no flipping on TL2 here
    end

Eq__(2) which can be re-written like

abcd_in(:,:,j) = inv(abcd_TL1(:,:,j)) * abcd_in(:,:,j) * inv(abcd_TL2(:,:,j))  



Do you think the skrf version is a mistake?


mhuser commented 2 years ago

Hi @denzchoe , thank you a lot for your code reviews, that make the IEEEP370 port better and stronger!

I have to admit translating this equation did make me sweat a lot. There is two different operator there : the matlab matrix left divide mldivide, \ and matrix right mrdivide, / here.

All your conclusions are correct:

All matrices are squares (abcd parameters at a single frequency) in a loop, so: mldivide solve x = A\B with A*x = B this is roughly equivalent to inv(A)*B

mrdivide solve x = B/A with x*A = B this is roughly equivalent to B*inv(A)

As you mentionned, we can then rewrite equ__(2) abcd_TL1 \ abcd_in / abcd_TL2 as inv(abdc_TL1)*abcd_in*inv(abcd_TL2)

We also know that B/A = (A'\B')' in which case this would be equ__(2) abcd_TL1 \ abcd_in / abcd_TL2 as (inv(abcd_TL2')*(inv(abcd_TL1)*abcd_in)')'

In both cases, there is no flip involved at all.

However, the way the error box grows from external port to the inside of networks and errorbox2 is flipped at the end of the process, it seemed logical to me (in a moment of bewilderment or stupefaction) to flip TL2 accordingly.

This being said : TL1 and TL2 are symmetrical transmission line devices, so this should not have any influence on the results

It looks clear to me now it would be closer to the initial source and more computation efficient to remove the flip and maybe add a comment stating that TL2 is symmetrical so no flip is required.

denzchoe commented 2 years ago

This being said : TL1 and TL2 are symmetrical transmission line devices, so this should not have any influence on the results

Yes I accidentally ignored this when cross-checking. It seems that the ZC recreate the TxLine for the errorbox to use. Guess there is no issue then.

I have to admit translating this equation did make me sweat a lot.

I agree, me too. haha. I wonder why the rigidity in the MATLAB TG1 code. Last I remember, there was the inv( ) available on it. Ok I just checked. Looks like it's mainly for computational efficiency reasons

We also know that B/A = (A'\B')' in which case this would be equ__(2) abcd_TL1 \ abcd_in / abcd_TL2 as (inv(abcd_TL2')*(inv(abcd_TL1)*abcd_in)')'

This equation is making me sweat all over again. @__@

However, the way the error box grows from external port to the inside of networks and errorbox2 is flipped at the end of the process, it seemed logical to me (in a moment of bewilderment or stupefaction) to flip TL2 accordingly.

When you first shared your alpha code version of the porting, I had a hard-time recreating your observations of the side1**side2 == s2xthru. Because I was so use to the skrf way of doing things, i.e.: side1**side2.flipped( ) After tens of minutes, it then hit me.. wait.. MATLAB, ISD (De-embedding Software 1) & AFR (De-embedding Software 2) export the S-Parameters Port numbering differently.

My opinion in addition to what you have mentioned here in PR#694

IMO, I am not sure whether it is a good thing or a bad thing to modify the port numbering method. Because during plot labelling at Legend, it is going to look a little extra should we plot the residue after deembedding, i.e.: label will have the following context side1.inv ** s2xthru ** side2.flipped( ).inv Following the general standards (skrf) is good too. (Just like the rest of the RTD examples.) So.. 🤷

mhuser commented 2 years ago

With issue https://github.com/scikit-rf/scikit-rf/issues/697 my proposition is to stick with the port numbering scheme recommended in the standard.

It probably will be always quite ambiguous, so following the standard we are implementing is probably the best choice 😉