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

Add gain properties #908

Merged
merged 12 commits into from
May 13, 2023
Merged

Add gain properties #908

merged 12 commits into from
May 13, 2023

Conversation

keikawa
Copy link
Contributor

@keikawa keikawa commented May 6, 2023

The maximum stable gain, available gain, and Mason's unilateral gain were added to the Network object.

@jhillairet jhillairet added the New Feature A new feature in progress label May 6, 2023
Copy link
Member

@jhillairet jhillairet left a comment

Choose a reason for hiding this comment

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

Thanks for contributing!

I've made some suggestion on the documentation, to be discussed.

Would you mind adding some associated tests eventually?

skrf/network.py Outdated Show resolved Hide resolved
skrf/network.py Outdated Show resolved Hide resolved
skrf/network.py Outdated Show resolved Hide resolved
skrf/network.py Show resolved Hide resolved
@keikawa
Copy link
Contributor Author

keikawa commented May 6, 2023

@jhillairet
Thank you very much for review and suggestion!
I would like to add a test code, please wait a little while longer.

@Ttl
Copy link
Collaborator

Ttl commented May 7, 2023

I fixed some issues with stability in #910 when S12 or S21 is zero. Can you make sure that these methods also work correctly when S12 or S21 are zero? Also I would prefer raising ValueError to assert.

@keikawa
Copy link
Contributor Author

keikawa commented May 7, 2023

Nice catch. I need to fix zero division issues in the gain properties.

@jhillairet
Could you firstly merge #910 to the master branch bacause the gain properties depend on the stability?

@jhillairet
Copy link
Member

@jhillairet Could you firstly merge #910 to the master branch bacause the gain properties depend on the stability?

OK, merged.

@keikawa
Copy link
Contributor Author

keikawa commented May 7, 2023

I added test codes and chaged assert to ValueError.
For the zero division case, the gain properties return npy.inf or npy.nan with runtime warning (This operaiton is depend on Numpy).
Please advise me if you have any better way other than above.

Copy link
Member

@jhillairet jhillairet left a comment

Choose a reason for hiding this comment

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

Great, thank you. Small additionnal comments:

  • could you comment your tests to explain in a few words their purpose?
  • Eventually, if you could reduce the number of points of the test files (fet.s2p, maxgain_ads.csv). ~hundred of frequency points is enough for testing and it saves some space.

skrf/tests/test_network.py Outdated Show resolved Hide resolved
@Ttl
Copy link
Collaborator

Ttl commented May 9, 2023

nan is inconvenient return value because of special properties of it (nan != nan). It's nicer to return inf instead when that makes sense. You can check what I did in #910. For example in msg:

    infs = npy.full(self.s.shape[0], npy.inf)
    abs_s12 = npy.abs(self.s[:, 0, 1])
    gms = npy.divide(npy.abs(self.s[:, 1, 0]), abs_s12, out=infs, where=abs_s12!=0)
    return gms

max_gain should also handle the case where stability returns inf. This formulation also has problems with precision when k is large, see here for alternate formula that should be numerically better: https://www.microwaves101.com/encyclopedias/stability-factor

@keikawa
Copy link
Contributor Author

keikawa commented May 9, 2023

Thank you for comment!

@Ttl
In my first commit, I used this equation for max_gain calculation.
Nevertheless, I changed the equation with no reason in the second commit.
I seem to have been writing code in my sleep...

By the way, does inf really make sense as a return value for max_stable_gain?
If S12 = S21 = 0, msg is not always infinity.
Wouldn't it be more user-friendly to have nan as the return value like the default behavior of Numpy?

@Ttl
Copy link
Collaborator

Ttl commented May 9, 2023

That's true in that case. If it handles S12 = 0, S21 != 0 correctly now then it's fine.

@keikawa
Copy link
Contributor Author

keikawa commented May 10, 2023

I understand that the default behavior of Numpy is to return inf when dividing a finite value by 0, and nan when dividing 0 by 0.
In either case, a runtimeWarning is printed.

The current test only detects runtime warnings, do I need to check inf and nan to see if they are output correctly?
I'm not familiar with testing, so if there is a better way, please let me know.

@jhillairet jhillairet merged commit d97735c into scikit-rf:master May 13, 2023
11 checks passed
@jhillairet
Copy link
Member

Thank you @keikawa

I forgot to merge this PR before merging the new version, I'll make a minor version to integrate it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Network New Feature A new feature in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants