Skip to content
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.

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

Resolve singularities for matched thru and line in determine_reflect #870

Merged
merged 14 commits into from
Apr 19, 2023

Conversation

eendebakpt
Copy link
Contributor

Fixes #865

This PR allows the determine_reflect (used in TRL calibration) to run a matched thru and line. The approach is inspired by the comment #865 (comment) from @Ttl

@Vinc0110 Vinc0110 marked this pull request as draft April 1, 2023 12:26
@eendebakpt eendebakpt force-pushed the determine_reflect branch 3 times, most recently from 25efc75 to ef10ac9 Compare April 2, 2023 20:24
@eendebakpt eendebakpt marked this pull request as ready for review April 3, 2023 08:31
Copy link
Collaborator

@Ttl Ttl left a comment

Choose a reason for hiding this comment

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

I tested the changes and it seems to work fine. No issues with the functionality, but few code quality comments.

skrf/calibration/calibration.py Outdated Show resolved Hide resolved
skrf/calibration/tests/test_calibration.py Outdated Show resolved Hide resolved
skrf/calibration/tests/test_calibration.py Outdated Show resolved Hide resolved
skrf/calibration/tests/test_calibration.py Outdated Show resolved Hide resolved
skrf/calibration/tests/test_calibration.py Outdated Show resolved Hide resolved
skrf/calibration/tests/test_calibration.py Show resolved Hide resolved
skrf/calibration/calibration.py Show resolved Hide resolved
skrf/calibration/calibration.py Outdated Show resolved Hide resolved
skrf/calibration/calibration.py Outdated Show resolved Hide resolved
skrf/calibration/calibration.py Outdated Show resolved Hide resolved
@eendebakpt
Copy link
Contributor Author

@Ttl When making the review changes I found a problematic case. I could solve the case by increasing epsilon in the regularization from 1e-7 to 1e-6, but I am not familiar enough with the material to judge whether 1e-6 is too large.

I changed the root selection criterium to one from the original TRL paper. It works better for the cases I tested, but perhaps we need some more test cases.

On current master the case from the regression test results in NaN values in the outcome.

@eendebakpt eendebakpt changed the title Draft: resolve singularities for matched thru and line in determine_reflect Resolve singularities for matched thru and line in determine_reflect Apr 13, 2023
@eendebakpt eendebakpt requested a review from Ttl April 13, 2023 19:55
rootChoice = [abs(x1[i] - e2[i]) < abs(x2[i] - e2[i]) for i in range(len(x1))]
e2 = line.s[:,0,1]**2
rootChoice0 = abs(x1 - e2) < abs(x2 - e2) # original choice for the root. see gh-870
rootChoice = npy.abs(x1) < npy.abs(x2) # criterium from eq (55)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This chooses the root based on which one gives lossy line. However it is inaccurate for very low loss line such as waveguide. The original one is also described in the paper and is more robust. Keep the old root choice algorithm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Ttl I reverted the root choice, but now I have a failing test which I do not know how to fix. The failing test is:

        freq = rf.F(434615384.6153846e-9, .7, 1, unit = 'GHz')
        medium = Coaxial.from_attenuation_VF(freq, att = 3.0, VF = .69)
        thru= medium.line(0, 'm')
        line = medium.line(0.12, 'm')
        short = rf.Network(f=freq.f, s=[[[(-1.+0.017870117714376983j)] ] ])

        r = determine_reflect(thru, rf.two_port_reflect(short, short), line)
        npy.testing.assert_array_almost_equal( r.s, short.s)

It does pass with the rootChoice = npy.abs(x1) < npy.abs(x2) test, but not with the rootChoice = abs(x1 - e2) < abs(x2 - e2). Do you have any suggestions on how to address this? (maybe this is a case that we should not expect to work, but it seems like a reasonable case to me)

Copy link
Collaborator

Choose a reason for hiding this comment

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

This passes on current master:

    def test_determine_reflect_regression(self):
        freq = rf.F(434615384.6153846e-9, .7, 1, unit = 'GHz')
        medium = Coaxial.from_attenuation_VF(freq, att = 3.0, VF = .69)
        thru = medium.line(0, 'm')
        line = medium.line(0.12, 'm')
        short = medium.load(-1.+0.017870117714376983j)

        # workaround division by zero
        thru.s[:,0,0] += 1e-7
        thru.s[:,1,1] += 1e-7
        r = determine_reflect(thru, rf.two_port_reflect(short, short), line)
        npy.testing.assert_array_almost_equal(r.s, short.s)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Ttl Ok. I tried before only to regularize t, but with the regularization on s now all tests are passing.

skrf/calibration/tests/test_calibration.py Outdated Show resolved Hide resolved
skrf/calibration/tests/test_calibration.py Show resolved Hide resolved
skrf/calibration/tests/test_calibration.py Outdated Show resolved Hide resolved
skrf/calibration/tests/test_calibration.py Outdated Show resolved Hide resolved
@jhillairet jhillairet merged commit cf292d9 into scikit-rf:master Apr 19, 2023
10 checks passed
@jhillairet jhillairet mentioned this pull request May 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue with TRL and determine_reflect
4 participants