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

Support heterogeneous parameters for linearize and remake #2283

Merged
merged 17 commits into from
Oct 4, 2023

Conversation

YingboMa
Copy link
Member

@@ -1274,14 +1275,34 @@ See also [`linearize`](@ref) which provides a higher-level interface.
function linearization_function(sys::AbstractSystem, inputs,
outputs; simplify = false,
initialize = true,
op = Dict(),
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a very bad interface, now the user has to provide both p and op even if op already contains p. The new changes are also lacking documentation, how is the user supposed to know that they now have to provide also p for the results to be correct?

I would prefer to think a bit harder about this design, the current state feels like a quick hack that is going to be very confusing to use

Copy link
Member Author

Choose a reason for hiding this comment

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

The alternative is being wrong though.

Copy link
Member Author

Choose a reason for hiding this comment

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

The op here doesn't require the correct numerical value. It only cares about types.

Copy link
Contributor

Choose a reason for hiding this comment

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

The op here doesn't require the correct numerical value. It only cares about types.

What does this mean? op is the user-provided operating point around which to linearize.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since we use heterogenous parameters by default now, we need to know the exact type of the parameters to create the linearized function correctly.

@YingboMa YingboMa changed the title Suppose heterogeneous parameters for linearize and remake Support heterogeneous parameters for linearize and remake Sep 29, 2023
@YingboMa YingboMa force-pushed the bgc/split_params_bug branch from 360ddb3 to 2a553a7 Compare October 2, 2023 18:15
@YingboMa YingboMa merged commit 2309f9f into master Oct 4, 2023
26 of 33 checks passed
@YingboMa YingboMa deleted the bgc/split_params_bug branch October 4, 2023 22:02
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