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 true-false-comparison errors. #937

Merged
merged 2 commits into from
Jul 28, 2023
Merged

Conversation

bchpmn
Copy link
Contributor

@bchpmn bchpmn commented Jul 22, 2023

Commit 1228661, 'Rule E712: true-false-comparison', replaced boolean comparison == with boolean comparison by id is in some conditional statements which results in exceptions not raised and unexpected behavior.

The commit attached replaces comparison by id fn() is False with truthy true/false not fn() for conditional logic that compares the result of a callable to a boolean. It restores the original behavior (<=v0.26.0) of the modified functions.

Cascading two networks with different frequency data is shown below to demonstrate the issue.

(<=v0.26.0, attached):

The original code / attached changes return a network with the intersecting subset of frequency data.
skrf.connect raises an IndexError by way of skrf.check_frequency_equal if the frequency data is not equal and no common values are found with np.intersect1d. If some common values are found, a warning alerts about dropped data.

>>> import skrf as rf, numpy as np
>>> a = rf.Network(f=[1,2,3], s=np.zeros((3,2,2),dtype='c16'))
>>> b = rf.Network(f=[4,5,6], s=np.ones((3,2,2),dtype='c16'))
>>> c = rf.Network(f=[1,3,5], s=np.ones((3,2,2),dtype='c16'))
>>> (a ** b).f
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/bec/scikit-rf/skrf/network.py", line 551, in __pow__
    return cascade(self, other)
  File "/home/bec/scikit-rf/skrf/network.py", line 4669, in cascade
    return connect(ntwkA, N, ntwkB, 0, num=N)
  File "/home/bec/scikit-rf/skrf/network.py", line 4355, in connect
    raise e
  File "/home/bec/scikit-rf/skrf/network.py", line 4351, in connect
    check_frequency_equal(ntwkA, ntwkB)
  File "/home/bec/scikit-rf/skrf/network.py", line 7140, in check_frequency_equal
    raise IndexError('Networks don\'t have matching frequency. See `Network.interpolate`')
IndexError: Networks don't have matching frequency. See `Network.interpolate`
>>> (a**c).f
/home/bec/scikit-rf/skrf/network.py:4359: UserWarning: Using a frequency subset:
1.0-3.0 GHz, 2 pts
  warnings.warn("Using a frequency subset:\n" + str(ntwkA.frequency))
array([1.e+09, 3.e+09])

(v0.27.1, v0.28.0):

In the current release skrf.connect always returns frequency data from the first input as a result of the conditional
skrf.assert_frequency_equal is False never being satisfied in the function skrf.check_frequency_equal and not raising the IndexError which is caught in skrf.connect to determine the shared subset of values.

Python 3.10.6 (main, May 29 2023, 11:10:38) [GCC 11.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import skrf as rf, numpy as np
>>> a = rf.Network(f=[1,2,3], s=np.ones((3,2,2), dtype='c16'))
>>> b = rf.Network(f=[4,5,6], s=np.zeros((3,2,2), dtype='c16')) 
>>> c = rf.Network(f=[1,3,5], s=np.zeros((3,2,2), dtype='c16')) 
>>> (a**b).f
array([1.e+09, 2.e+09, 3.e+09])
>>> (a**c).f
array([1.e+09, 2.e+09, 3.e+09])

Replace boolean comparison by id ("fn() is False") with truthy true/false
("not fn()") in conditional logic that compares the result of a callable
and boolean. Commit restores original behavior of skrf.connect for networks
that do not have identical frequency data.
@jhillairet jhillairet added the fix Bug Fix label Jul 23, 2023
@jhillairet
Copy link
Member

how indeed, good catch!

Did you look if there are more "is True" or "is False" in the codebase which could bring similar problems?

@bchpmn
Copy link
Contributor Author

bchpmn commented Jul 23, 2023

I did grep around. The remaining use of is True|False compares a boolean variable which should work as intended.

bec@dsktop:~/scikit-rf$ grep -HEnr "is\ +True|is\ +False" skrf
skrf/vectorFitting.py:459:                if converged and stop is False:
skrf/plotting.py:186:    elif draw_vswr is True:
skrf/network.py:2394:            if return_string is True or type(to_archive) is zipfile.ZipFile:
skrf/network.py:2555:            elif return_string is True:
skrf/io/general.py:325:    if sort is True:
skrf/io/general.py:635:    if remove_tmp_file is True:

note: Comments and other irrelevant matches for is True or is False were manually removed from the output

The last function may have a bug in the case that remove_tmp_file is never set?

@jhillairet
Copy link
Member

The last function may have a bug in the case that remove_tmp_file is never set?

Possibly. Up to you if you want to correct it now or not. If not I'll do it after.

@github-actions github-actions bot added the IO label Jul 26, 2023
@jhillairet jhillairet merged commit 8f0d537 into scikit-rf:master Jul 28, 2023
11 checks passed
@jhillairet
Copy link
Member

Thank you @bchpmn

@jhillairet jhillairet mentioned this pull request Sep 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants