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

Remove stale connect and use named arguments #97

Closed
wants to merge 1 commit into from
Closed

Remove stale connect and use named arguments #97

wants to merge 1 commit into from

Conversation

mzaffalon
Copy link
Contributor

Update tests accordingly

@mzaffalon
Copy link
Contributor Author

mzaffalon commented Oct 6, 2023

I took the liberty to remove the old signature connect and move the body into the new connect.

I have also added properly named keyword arguments: I found it brittle to having the variable names w1 and 'z1' forced on the user. If you agree, I can rename those variables: if there is a variable called w1 I ask myself if I should also look for a variable w0 or w2.

The example complicated_feedback.jl is run twice: is this intentional?

@baggepinnen
Copy link
Member

I cannot merge this PR, the new signature was added but the old one was kept in place to not break code that used it. Me and colleagues still have code around that uses it.

I also don't want to break the interface by removing the z1/w1 keyword arguments, and these are consistent with the corresponding arguments to the lower-level feedback function in ControlSystems.jl
image

The choices z,y,w,u are also consistent with the robust control literature, for example "ROBUST AND OPTIMAL
CONTROL" by ZHOU, DOYLE and GLOVER, and I would prefer to keep them that way.
image

The example complicated_feedback.jl is run twice: is this intentional?

Nope, that's probably a mistake

In general, breaking the interface in any way is something I avoid to the greatest extent possible, each breaking change causes a ton of downstream problems that I and other users cannot afford.

@mzaffalon
Copy link
Contributor Author

Thank you for the clear explanation.

And now I feel stupid not to have checked before that both connect([F, R, C, P, addP, addC], connections; w1=[:uF]) and

w1=[:uF]
connect([F, R, C, P, addP, addC], connections; w1)

work. In this case I am fine with the interface as it is.

Sorry for wasting your time.

@mzaffalon mzaffalon closed this Oct 6, 2023
@baggepinnen
Copy link
Member

No worries

Keyword arguments that have the same name as your variable do not require the repeating in (; w1=w1). This is true all over Julia, for example, in named tuple creation (; w1), struct unpacking etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants