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

Refactor media to simplify line creation #727

Merged

Conversation

mhuser
Copy link
Collaborator

@mhuser mhuser commented Jun 29, 2022

This PR is the second attempt to fix #651

Ready for review.

Todo

  • Refactor all media for z0_port, z0 scheme
  • Refactor waveguides media for z0_override, z0 scheme
  • Make all tests pass and refine
  • Make all notebooks build
  • Update notebooks (check contents)
  • Update documentation
  • Extend documentation (e.g. by passing z0 to Media.line())

@jhillairet jhillairet added this to the v1.0.0 milestone Nov 6, 2022
@mhuser mhuser closed this Apr 12, 2023
@mhuser mhuser force-pushed the refactor-media-to-simplify-line-creation branch from 9571e08 to f9928fe Compare April 12, 2023 20:33
@github-actions github-actions bot removed Calibration Documentation Request/Improvement of the documentation Media labels Apr 12, 2023
@mhuser mhuser reopened this Apr 12, 2023
@github-actions github-actions bot added Calibration Documentation Request/Improvement of the documentation Media labels Apr 12, 2023
@mhuser
Copy link
Collaborator Author

mhuser commented Apr 20, 2023

A deprecation warning is now emmited if z0 is used in Media initialization. This helped to find some more z0 remaining.
The z0 fallback on z0_port, resp. z0_override if these are None.

@mhuser
Copy link
Collaborator Author

mhuser commented Apr 24, 2023

After having reflected a bit on it, I think there is some tradeoff here. If we keep the PR to merge within 1.0, it probably will not benefits from the extensive testing by many users on Master, with the risk to miss key issues that we later have to fix in 1.0.
On the other hand, breaking user code can act as a repellent. Here they will always get a media line with a warning in the cases were the behaviour is subject to have changed.

@jhillairet
Copy link
Member

Updated documentation for reviewers:
https://scikit-rf--727.org.readthedocs.build/en/727/

  • doc/source/tutorials/Introduction.ipynb
  • doc/source/tutorials/Media.ipynb
  • doc/source/examples/circuit/Wilkinson Power Splitter.ipynb
  • doc/source/examples/interactive/Interact Transmission Lines.ipynb
  • doc/source/examples/interactive/Interactive Coaxial Properties.ipynb
  • doc/source/examples/interactive/Interactive Mismatched Line.ipynb
  • doc/source/examples/metrology/Calibration With Three Receivers.ipynb
  • doc/source/examples/metrology/LRRM.ipynb
  • doc/source/examples/metrology/Multi-port Calibration.ipynb
  • doc/source/examples/metrology/Multiline TRL.ipynb
  • doc/source/examples/metrology/NanoVNA_V2_4port-splitter.ipynb
  • doc/source/examples/metrology/SOLT Calibration Standards Creation.ipynb
  • doc/source/examples/metrology/TwoPortOnePath, EnhancedResponse, and FakeFlip.ipynb
  • doc/source/examples/networksets/Sorting Network Sets.ipynb
  • doc/source/examples/networktheory/Balanced Network De-embedding.ipynb
  • doc/source/examples/networktheory/CPW media example.ipynb
  • ...ce/examples/networktheory/Correlating microstripline model to measurement.ipynb
  • doc/source/examples/networktheory/DC Extrapolation for Time Domain .ipynb
  • doc/source/examples/networktheory/DefinedAEpTandZ0 media example.ipynb
  • doc/source/examples/networktheory/IEEEP370 Deembedding.ipynb
  • doc/source/examples/networktheory/LNA Example.ipynb
  • doc/source/examples/networktheory/Properties of Rectangular Waveguides.ipynb
  • ...amples/networktheory/Time domain reflectometry, measurement vs simulation.ipynb
  • ...rce/examples/networktheory/Transmission Line Properties and Manipulations.ipynb
  • doc/source/examples/taper/tapered_transmission_lines.ipynb

doc/source/tutorials/Media.ipynb Outdated Show resolved Hide resolved
skrf/media/circularWaveguide.py Outdated Show resolved Hide resolved
skrf/media/coaxial.py Outdated Show resolved Hide resolved
Comment on lines 166 to 172
"""
Characteristic Impedance.

Returns
-------
z0 : :class:`numpy.ndarray`
"""
Copy link
Member

Choose a reason for hiding this comment

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

I think additional explanation should be given about what is returned here (None or something)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now that z0_override is a property of the abstract class Media, the switch between z0_override and abstract method z0_characteristic can be done here. The media classes will need to define z0_characteristic, which sounds quite clear what it is 😉

    @property
    @abstractmethod
    def z0_characteristic(self):
        """
        Characteristic Impedance.
        This abstract method has to be defined in the Media Class.
        
        Returns
        -------
        z0_characteristic: npy.ndarray
            Characteristic Impedance in units of ohms
        """
        return None
    
    @property
    def z0(self):
        """
        Return Characteristic Impedance `z0_characteristic`.
        If `z0_override` is not None, it is returned instead.
        
        Returns
        -------
        z0 : npy.ndarray
            z0_characteristic or z0_override in units of ohms
        """
        if self.z0_override is None:
            return self.z0_characteristic
        else:
            return self.z0_override

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@jhillairet
Copy link
Member

@mhuser do you think it is ok for the next release now ? (or the one after the next one?)

@mhuser
Copy link
Collaborator Author

mhuser commented May 6, 2023

@mhuser do you think it is ok for the next release now ? (or the one after the next one?)

I think yes. Let's merge branch master inside. 😅

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 (and huge) work @mhuser . Let's see if anyone complain after the next release ;)

@jhillairet jhillairet merged commit 080a1dd into scikit-rf:master May 7, 2023
11 checks passed
@jhillairet jhillairet mentioned this pull request May 7, 2023
11 tasks
@mhuser mhuser deleted the refactor-media-to-simplify-line-creation branch May 7, 2023 15:27
@jhillairet jhillairet mentioned this pull request May 7, 2023
@drsvenkuehn
Copy link

This PR is the second attempt to fix #651

Ready for review.

Todo

  • Refactor all media for z0_port, z0 scheme
  • Refactor waveguides media for z0_override, z0 scheme
  • Make all tests pass and refine
  • Make all notebooks build
  • Update notebooks (check contents)
  • Update documentation
  • Extend documentation (e.g. by passing z0 to Media.line())

I believe this breaks the "media coaxial" example in the tutorial.

@jhillairet
Copy link
Member

@drsvenkuehn
Copy link

I believe this breaks the "media coaxial" example in the tutorial.

@drsvenkuehn, which one ? https://scikit-rf.readthedocs.io/en/latest/tutorials/Introduction.html#Coax ? or https://scikit-rf.readthedocs.io/en/latest/tutorials/Media.html#RG58C/U-coaxial-cable ?

@jhillairet
the RG58 one. print(f'z0_port = {rg58.z0_port[0]}') gives an error as z0_port[0] is gone. Also, the S-parameter results are not the same as before: rg58.line(100, unit = 'mm', z0 = 25, name = '100 mm, 25 Ohm') appears always perfectly matched independent of z0

@mhuser
Copy link
Collaborator Author

mhuser commented Nov 28, 2023

Hello @drsvenkuehn ,
Thank you for your interest in Media.
I am a bit confused because after looking at https://scikit-rf.readthedocs.io/en/latest/tutorials/Media.html#RG58C/U-coaxial-cable, I did not notice the behaviour that you describe.

print(f'z0_port = {rg58.z0_port[0]}')

gives z0_port = 50.0

and

rg58.line(100, unit = 'mm', z0 = 25, name = '100 mm, 25 Ohm')

Gives mismatch
tutorials_Media_12_0

Is it possible you are speaking of another version ?

@drsvenkuehn
Copy link

drsvenkuehn commented Dec 6, 2023

Hi @mhuser ...sorry, my bad. I didn't use the right virtual environment. I used an old version. I get the same results with 0.30.0

@jhillairet
Copy link
Member

Ok, thanks for letting us know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Calibration Documentation Request/Improvement of the documentation Media Network
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Creating a line with characteristic impedance of the media is confusing
3 participants