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

Using SI Units #935

Merged
merged 4 commits into from
Sep 4, 2023
Merged

Using SI Units #935

merged 4 commits into from
Sep 4, 2023

Conversation

jhillairet
Copy link
Member

  • use Hz instead of GHz as new default
  • use second instead of nanosecond as new default

@jhillairet jhillairet added Feature Request Wished Feature Improvements Improvements of existing feature labels Jul 21, 2023
@jhillairet jhillairet added this to the v1.0.0 milestone Jul 21, 2023
@github-actions github-actions bot added the Time label Jul 21, 2023
@jhillairet jhillairet mentioned this pull request Jul 21, 2023
11 tasks
@jhillairet
Copy link
Member Author

Any comments on this PR? Did I miss something somewhere?

@jhillairet jhillairet merged commit 24e3e60 into scikit-rf:master Sep 4, 2023
11 checks passed
@jhillairet jhillairet mentioned this pull request Sep 4, 2023
@jhillairet jhillairet deleted the dev/SI_units branch September 4, 2023 08:44
@capn-freako
Copy link
Contributor

What is the default frequency unit in v0.30.0, Hz or GHz?

@jhillairet
Copy link
Member Author

Since this PR has been merged and version 0.29.0, it should be SI (Hz). Did you meet an issue?

@capn-freako
Copy link
Contributor

Since this PR has been merged and version 0.29.0, it should be SI (Hz). Did you meet an issue?

Yes, but I think one of my co-developers has answered it:

Hi @capn-freako ,

If we see the touchstone.py file here & the Network read_touchstone call here ;

if I interpret it correctly; the S-Parameters frequency unit gets passed to the Network Object regardless of the SI unit standardization. e.g.: If your Touchstone file has the following header # GHz S DB R 50.0 , then the Network Object created will inherently carry the unit of GHz.

I'm checking to see if this is my problem, now.

Thanks for responding so quickly!
-db

@capn-freako
Copy link
Contributor

capn-freako commented Jan 13, 2024

Nope, that wasn't it.

I'm able to fix things, by making this change to my code:

@@ -1255,11 +1256,11 @@ def interp_s2p(ntwk, f):
     assert rs == cs, "Non-square Touchstone file S-matrix!"
     assert rs == 2, "Touchstone file must have 2 ports!"

-    extrap = ntwk.interpolate(f, fill_value="extrapolate", coords="polar", assume_sorted=True)
+    extrap = ntwk.interpolate(f*1e-9, fill_value="extrapolate", coords="polar", assume_sorted=True)
     s11 = cap_mag(extrap.s[:, 0, 0])
     s22 = cap_mag(extrap.s[:, 1, 1])
-    s12 = ntwk.s12.interpolate(f, fill_value=0, bounds_error=False, coords="polar", assume_sorted=True).s.flatten()
-    s21 = ntwk.s21.interpolate(f, fill_value=0, bounds_error=False, coords="polar", assume_sorted=True).s.flatten()
+    s12 = ntwk.s12.interpolate(f*1e-9, fill_value=0, bounds_error=False, coords="polar", assume_sorted=True).s.flatten()
+    s21 = ntwk.s21.interpolate(f*1e-9, fill_value=0, bounds_error=False, coords="polar", assume_sorted=True).s.flatten()

Is it possible that the interpolate() function is still using GHz as its default?
(I perused your source a bit and it doesn't look like that should be the case.
But, I'm not sure what else to suspect.)

I've checked the f argument.
And it is correct and has units: Hz.

@capn-freako
Copy link
Contributor

Sorry, please ignore my comments above.
The error is somewhere on my end. :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Request Wished Feature Improvements Improvements of existing feature Time
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants