Fixed invalid s-parameters from wave port. #1904
Conversation
QimingFlex
force-pushed
the
qiming/wave_port
branch
2 times, most recently
from
4d602ab to
b626689
Compare
QimingFlex
force-pushed
the
qiming/wave_port
branch
2 times, most recently
from
dd6c50b to
63daa4e
Compare
There was a problem hiding this comment.
I think it makes sense but @weiliangjin2021 or ideally @dmarek-flex should know better.
|
Thanks, this fix will work and help reverse user error. The only problem I find is that it kind of changes the Port setup behind the scenes. I much prefer the Validator approach that you did for the current integral, then how the sign of the voltage is changed in the port. I think a better fix for this problem is to add another validator to the port for the voltage integral that ensures the positive terminal (and not the negative terminal) of the voltage integral is inside the current loop. This should be easy for the AxisAligned versions of the integrals. More work for the Custom versions. It would also be nice if you could extend your current integral validator to handle the case of the Setting up these integrals correctly is a bit of a pain, which is also why we are going to try and automate these steps. |
|
@dmarek-flex I'm thinking the same way but I haven't figured out how we know if the terminal is positive. That's why I try to change it behind the scenes as the port impedance will tell us if the integral is correct or not. |
If pursuing that approach, I think you can just throw an error where you currently flip the sign. But as @dmarek-flex also mentioned, before we have the automatic approach, there will be lots of pain setting up those path. I'm thinking if flipping the sign of voltage (and consequently the impedance) is all we need when voltage path is of the wrong direction, we can just silently handle it internally? |
|
@weiliangjin2021 Yeah, agree it is a lot of pain setting these paths if seeking automation. I went through the formulations and am pretty sure that flipping the sign is all we need. Throwing an error where I flip the sign wouldn't be ideal I guess as this is in post-processing and would ask users to re-run the simulation. |
Determining which terminal is the positive one is straightforward using the sign parameter. For the |
That should work as a temporary fix until we improve other parts. |
| negative_real_Z = np.real(Z_numpy) < 0 | ||
| V_numpy = np.where(negative_real_Z, -V_numpy, V_numpy) | ||
| Z_numpy = np.where(negative_real_Z, -Z_numpy, Z_numpy) |
There was a problem hiding this comment.
Maybe add a sanity check to make sure that the sign choice is consistent over all frequencies. The impedance should not change sign as the frequency changes (at least for typical transmission lines)
tidy3d/plugins/smatrix/ports/wave.py
Outdated
| name = values.get("name") | ||
| if val.sign != direction: | ||
| raise ValidationError( | ||
| f"'current_integral' sign must match the '{name}' direction ({direction})." |
There was a problem hiding this comment.
add missing quotes around direction
tidy3d/plugins/smatrix/ports/wave.py
Outdated
| @pd.validator("current_integral", always=True) | ||
| def validate_current_integral_sign(cls, val, values): | ||
| """ | ||
| Validate that the sign of current_integral matches the direction. |
There was a problem hiding this comment.
add double single quotes around current_integral
|
After further discussion, let's keep it simple and just silently swap the voltage sign when the real part of the line impedance is found to be negative. Let's just at least implement the sanity check to make sure the sign is the same for all frequencies. |
QimingFlex
force-pushed
the
qiming/wave_port
branch
3 times, most recently
from
2be935c to
a5d961c
Compare
|
@QimingFlex please take a look at the changes I made and do a final check. |
| @@ -396,3 +397,16 @@ def plot( | |||
| arrowprops=ARROW_CURRENT, | |||
| ) | |||
| return ax | |||
|
|
|||
| @cached_property | |||
There was a problem hiding this comment.
I think the ordering of vertices alone is not enough to determine the current flow. For example, consider two ports placed in the x-y plane. If we have the port direction as '+', then the ccw orientation represents '+' and there is no problem. While if we have the port direction as '-', then the sign of the current should be '-' as well. But with this approach, it will be incorrectly determined as '+'.
There was a problem hiding this comment.
This function only determines the sign of the current integral. The port direction and current integral sign are still compared in the WavePort's validator. In your example, the validator will correctly determine that there is a mismatch.
There was a problem hiding this comment.
I just generalized the code you had before to include the CustomCurrentIntegral2D
There was a problem hiding this comment.
Ah I see, so this is a sign of the integral rather than the sign of current flow. The rest looks good, thanks!
|
@QimingFlex ok I think you can squash, rebase, and merge now! |
…or negative real parts
QimingFlex
force-pushed
the
qiming/wave_port
branch
from
48338e9 to
354887b
Compare
S-matrix is defined by:
where I is the current flows into the port, so I here introduced a invalidator to throw an error if the integral sign of
CurrentIntegralAxisAligneddoesn't match the port's direction. In the subsequent S-matrix computation, if the voltage path is inversely chosen, it results in a negative Z_R, leading to invalid S-matrix. To address this, we now flip the sign where a negative Z_R occurs, which corresponds to where a negative voltage is given. This yields the correct S-matrix.