Noise Wave Overhaul for Scikit-rf #596
Conversation
…e parameter to subnetwork growth functions
…rk, which can also be used to calculate the noise figure at each port
…ls (erroneous but allows one to keep working)
… noise parameters for any combination of ports
Noise dev
added example notebook for noise waves
github-actions
bot
added
Documentation
|
Thank you for contributing to scikit-rf! This is a big work at once, please let us know some time to review and give comments. Currently, the tests are failing. |
|
@mbe9a could you create a separate PR from the first commit only? It seems this are cosmetic changes (only?), so it would be easier to review the rest. |
|
@jhillairet sorry about that! A later commit must have broken the tests again. I will continue to work through them. |
|
@FranzForstmayr I may know microwaves, but I am a GitHub novice. What's the best way to create a pull-request from a specific commit? I've seen examples where I could create a new branch and cherry pick commits. Would that work? |
|
You could checkout the commit and create a new branch from it. Then you could create a PR from this new branch. Then create a second PR :) |
|
The documentation you've made is great, thank you! I would suggest splitting the examples if different notebooks, and eventually to make the introductory parts in the /tutorial section of the documentation and referring to the examples. But that's very debatable. Also, the PR includes MAC-OS dir files (.DS_Store). Could you remove them? You can add the line:
to the |
| """ | ||
| """ |
There was a problem hiding this comment.
could you complete the docstring?
| """ | ||
| checks if two Networks have same noise frequency | ||
| """ |
There was a problem hiding this comment.
could you complete the docstring with the argument section?
| ((z01.conj() + s[:, 0, 0] * z01) * (z02.conj() + s[:, 1, 1] * z02) - s[:, 0, 1] * s[:, 1, | ||
| 0] * z01 * z02) / denom, |
There was a problem hiding this comment.
the way you have reformatted the line is a bit messy to my opinion.
| @@ -0,0 +1 @@ | |||
| /Applications/Xcode.app/Contents/Developer/Library/Frameworks/Python3.framework/Versions/3.8/Python3 | |||
There was a problem hiding this comment.
this file also should not be added
|
|
||
| if __name__ == "__main__": | ||
| unittest.main() |
There was a problem hiding this comment.
I think this is redundant with the line above, no?
| dtype: str | ||
| In order to use the subnetwork growth functions symbolically, dtype must be set to 'object'. |
There was a problem hiding this comment.
I wonder in which case you run into trouble to be obliged to add this parameter. Is it really necessary?
| def nf_w(self, Ta: float = T0, dB: bool = False) -> npy.ndarray: | ||
| """ |
There was a problem hiding this comment.
the parameters does not match with the docstring description
| ## Functions operating on Network[s] | ||
| def connect(ntwkA: Network, k: int, ntwkB: Network, l: int, num: int = 1) -> Network: | ||
| def connect(ntwkA: Network, k: int, ntwkB: Network, l: int, num: int = 1, tol: float = mf.ALMOST_ZERO) -> Network: |
There was a problem hiding this comment.
the new tol parameter should be add to the docstring
Hello!
I have a bit of an update for skrf that I hope you all will find valuable. I will say that my implementation is probably far from optimal, but for the initial pull-request I decided to add support for noise waves completely in parallel to the network noise already present in skrf. I still need to add test cases for the additions and comment more thoroughly, but my code has not broken anything as far as I know.
A quick summary of what I've added:
I have added a notebook under doc/source/examples/noisewaves/ that goes over all of this thoroughly and benchmarks results with ADS. There is a brief explanation of noise waves if you are unfamiliar. Noise waves are very naturally represented alongside s-parameters and are amenable to signal flow graph theory and subnetwork growth algorithms.
Please give me any thoughts you have. The implementation is a bit 'clunky' as I've added a lot of parameters that were already present but added a '_w' for 'waves' :) at the end. I have not added support for noise waves to everything but the main functions like connect, innerconnect, resample, renumber, etc have been modified.