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 de-embedding when 2 networks are provided #971

Merged
merged 3 commits into from
Nov 14, 2023

Conversation

atavella
Copy link
Contributor

@atavella atavella commented Nov 4, 2023

The network operator "//" used for de-embed 1 or 2 networks is not working for exactly 2 networks.

ntwk1 // ntwk2 works
ntwk1 // (ntwk2) works
ntwk1 // (ntwk2, ntwk3) does not work

it fixes that

@FranzForstmayr
Copy link
Collaborator

I think we should even be more strict and raise a Exception (RuntimeError?) if more than two elements are passed for other.

@atavella
Copy link
Contributor Author

@FranzForstmayr that makes sense to me. But, right now is raising a RuntimeWarning if more than two elements are passed and the flow continues with just the first two elements.

@FranzForstmayr
Copy link
Collaborator

Yes but warnings are likely to get overseen, and there's no reason to accept more than two for others.
If some existing code triggers the warning, there's probably another issue in the code which is simply not found yet.

@atavella
Copy link
Contributor Author

It seems to me that ValueError could be more suitable based on "Raised when an operation or function receives an argument that has the right type but an inappropriate value"

@jhillairet jhillairet added Bug Confirmed Bug fix Bug Fix labels Nov 14, 2023
@FranzForstmayr
Copy link
Collaborator

Thanks @atavella

@FranzForstmayr FranzForstmayr merged commit b32ff9c into scikit-rf:master Nov 14, 2023
12 checks passed
@jhillairet jhillairet mentioned this pull request Dec 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Confirmed Bug fix Bug Fix Network
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants