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

Fix incorrect documentation of offset loss unit for Open/Short/Load Standards #64

Closed
wants to merge 1 commit into from

Conversation

biergaizi
Copy link
Contributor

The documentation of offsetLoss in OpenStandard, ShortStandard and LoadStandard appears incorrect. It reads:

offsetLoss (optional) float loss due to skin-effect defined in Gohms/s at f0 (defaults to 0).

However, all of these classes actually pass the offsetLoss unchanged into the Offset class.

class OpenStandard(SParameters):
    """class providing the s-parameters of an open standard as commonly defined
    for a calibration kit."""
    def __init__(self,f,offsetDelay=0.0,offsetZ0=50.0,offsetLoss=0.0,f0=1e9,
                 C0=0.0,C1=0.0,C2=0.0,C3=0.0):
        # [...]
        offsetSParameters=Offset(f,offsetDelay,offsetZ0,offsetLoss,f0)

And the Offset class actually interprets offsetLoss in ohms/s, not Gohms/s.

offsetLoss (optional) float loss due to skin-effect defined in ohms/s at f0 (defaults to 0).

Due to this typo, I've wasted hours trying to solve my incorrect calculations in SignalIntegrity, it was because offsetLoss didn't do anything - it was too small by 9 orders of magnitude.

This patch fixes the documentation.

…tandards

Signed-off-by: Yifeng Li <tomli@tomli.me>
@PetePupalaikis
Copy link
Contributor

This branch has been merged into the InNextRelease branch and will go in the next release.

Sorry for the error. I actually never have arguments in Giga, or Mega or anything like that. I just wasn't thinking straight when I wrote the documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants