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

Fixed __eq__ issue #915

Merged
merged 8 commits into from
Jun 26, 2023
Merged

Fixed __eq__ issue #915

merged 8 commits into from
Jun 26, 2023

Conversation

Hadimius
Copy link
Contributor

@Hadimius Hadimius commented Jun 6, 2023

Changed eq so it accounts for differences in frequency and all primary properties. Refers to and solves issue #914

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 contribution, thank you!

@Ttl
Copy link
Collaborator

Ttl commented Jun 7, 2023

Comparing all PRIMARY_PROPERTIES is unnecessary. Network object stores only S-parameters and other parameters are converted from them as necessary. It's enough to compare f, s, s_def and z0. s_def comparison is only necessary if any of z0 has non-zero imaginary component. Different network with the same real z0 and s are equal even if s_def is different. Also noise parameters should be compared if they exist.

numpy will raise error in future when comparing two different length arrays by equality. It would be better to first compare the number of frequency points. Tests would be nice to have to check all these different cases.

@Vinc0110 Vinc0110 linked an issue Jun 7, 2023 that may be closed by this pull request
@jhillairet
Copy link
Member

I agree that comparing all properties is maybe overkill. Is the more the better?

Or should we really restrict the comparisons to:

  • f
  • s
  • z0
  • s_def (if any of z0 has non-zero imaginary component)

for noise parameter, eventually too.

skrf/network.py Outdated
if self.s_def == other.s_def:
return True
else:
if npy.all(npy.abs(npy.real(self.z0)) - npy.abs(npy.real(other.z0)) < ZERO):
Copy link
Collaborator

Choose a reason for hiding this comment

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

The abs is at the wrong place.
Maybe use something like return npy.allclose(self.z0.real, other.z0.real, atol=ZERO)


f2 = npy.arange(11)
n2 = rf.Network(s=s,f=f2)
self.assertFalse(n1 == n2)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe one more test with f2 = f1 * 10, as this test returns when checking the length of the vectors.

@Hadimius
Copy link
Contributor Author

It seams to be failing because of the following test in test_media:

    def test_impedance_mismatch(self):
        """
        Test the naming of the network. When circuit is used to connect a
        topology of networks, they should have unique names.
        """
        name = 'impedanceMismatch,50to25'
        qucs_ntwk = Network(os.path.join(self.files_dir, name + '.s2p'))
        self.dummy_media.frequency = qucs_ntwk.frequency
        skrf_ntwk = self.dummy_media.thru(z0=50, name = name)**\
            self.dummy_media.thru(z0=25)

        self.assertEqual(qucs_ntwk, skrf_ntwk)
        self.assertEqual(qucs_ntwk.name, skrf_ntwk.name)

I'm not 100% sure about this, but the Networks in this test should not be equal, since they differ in z0, or?
So should the test be adapted to compare only f, s and name?

@FranzForstmayr
Copy link
Collaborator

I think the problem is, that the file stored in sNp has always the same port impedance on all ports, in this case 50.
You cannot store 50/25 Ohm in sNp, so this test has to fail by definition.

You could manually set qucs_ntwk.z0 to [50, 25] to avoid this issue. If you do, please write a small note :)

@github-actions github-actions bot added the Media label Jun 19, 2023
@FranzForstmayr
Copy link
Collaborator

FranzForstmayr commented Jun 19, 2023

@Hadimius don't worry about CI, this issue is from the latest numpy release from yesterday. We need to fix this ASAP :)

@FranzForstmayr
Copy link
Collaborator

Can you merge in master please?

@FranzForstmayr
Copy link
Collaborator

Looks good!
Thank you @Hadimius for your effort!

@FranzForstmayr FranzForstmayr merged commit ab41997 into scikit-rf:master Jun 26, 2023
20 checks passed
@jhillairet jhillairet mentioned this pull request Jun 26, 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.

__eq__ does not compare frequency
4 participants