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

Allow NISTMultilineTRL to work with non-exact floats #895

Merged
merged 11 commits into from
Apr 19, 2023

Conversation

eendebakpt
Copy link
Contributor

@eendebakpt eendebakpt commented Apr 13, 2023

The NISTMultilineTRL checks whether variables are scalar with the construction

try:
   v = x[0]
except TypeError:
    # treat x as scalar
    ...

This works for float, since v = 1.0; v[0] results in a TypeError. But for v = npy.float64(1.0); v[0] an IndexError is raised. This PR allows NISTMultilineTRL to work with numpy scalars by checking arguments with numpy.isscalar and converting if required. The PR also improves performance of s2t by factoring out a common expression.

Copy link
Collaborator

@Ttl Ttl left a comment

Choose a reason for hiding this comment

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

I would prefer making them arrays in __init__ so that try except is not necessary. npy.isscalar could be useful for it as it handles both numpy types and ordinary python floats.

Just adding a comment is not good enough fix for #889. I'll submit a PR for it later.

Copy link
Collaborator

@Ttl Ttl left a comment

Choose a reason for hiding this comment

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

Looks mostly fine, just few comments.

skrf/calibration/calibration.py Outdated Show resolved Hide resolved
@@ -3163,20 +3158,20 @@ def solve_A(B1, B2, CoA1, CoA2):
raise ValueError('Only one of c0 or z0_line can be given.')
try:
c0m = self.c0[m]
except TypeError:
except (TypeError, IndexError):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this also be changed to use isscalar instead of try ... else? Either at __init__ set it to array or None or check it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to convert scalar values to a list in the __init__. It takes a bit extra memory, but I think it is rare for the frequency to be so large this causes problems.

Note that the z0_line, etc. are public attributes so this slightly changes the public behavior of the TRL.

skrf/calibration/calibration.py Outdated Show resolved Hide resolved
skrf/calibration/tests/test_calibration.py Outdated Show resolved Hide resolved
skrf/calibration/tests/test_calibration.py Outdated Show resolved Hide resolved
@jhillairet jhillairet merged commit ddd0a0a into scikit-rf:master Apr 19, 2023
10 checks passed
@jhillairet jhillairet mentioned this pull request May 6, 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.

None yet

3 participants