Skip to content

refactor(Blocks): add RealInputArray and RealOutputArray #283

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Apr 18, 2024
Merged

refactor(Blocks): add RealInputArray and RealOutputArray #283

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
633823c
refactor(Blocks): add `isarray` kwarg in RealInput/Output to have the…
Apr 12, 2024
6bf4b5d
refactor: add RealInputArray and RealOutputArray
Apr 15, 2024
4cfc041
refactor: change start values as guesses rather than default values f…
Apr 16, 2024
96b6de6
refactor: make nin and nout required for Real*Array components
Apr 16, 2024
77f3014
test: add in the missing values for Blocks.get_looptransfer tests
Apr 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Blocks/Blocks.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import IfElse: ifelse
import ..@symcheck
using ModelingToolkit: getdefault, t_nounits as t, D_nounits as D

export RealInput, RealOutput, SISO
export RealInput, RealInputArray, RealOutput, RealOutputArray, SISO
include("utils.jl")

export Gain, Sum, MatrixGain, Feedback, Add, Add3, Product, Division
Expand Down
58 changes: 50 additions & 8 deletions src/Blocks/utils.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
@connector function RealInput(; name, nin = 1, u_start = nin > 1 ? zeros(nin) : 0.0)
nin > 1 && @warn "For inputs greater than one, use `RealInputArray`."
if nin == 1
@variables u(t)=u_start [
input = true,
Expand All @@ -14,19 +15,40 @@
ODESystem(Equation[], t, [u...], []; name = name)
end
@doc """
RealInput(;name, nin, u_start)
RealInput(;name, u_start)

Connector with one input signal of type Real.

# Parameters:
- `nin=1`: Number of inputs
- `u_start=0`: Initial value for `u`
- `u_start=0`: Initial value for `u`.
Copy link
Member

Choose a reason for hiding this comment

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

Why does this have an initial value default?

Copy link
Member Author

Choose a reason for hiding this comment

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

Shouldn't there be a default value for the variable?

julia> @named x = RealInput(; nin = 1, u_start = 1.0)
Model x with 0 (1) equations
Unknowns (1):
  u(t) [defaults to 1.0]: Inner variable in RealInput x
Parameters (0):

Copy link
Member

Choose a reason for hiding this comment

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

Won't that cause issues in initialization?

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed in 4cfc041

Copy link
Member Author

Choose a reason for hiding this comment

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

Removing the default is erroring - https://github.com/SciML/ModelingToolkitStandardLibrary.jl/actions/runs/8706828508/job/23880355450?pr=283#step:6:577 even though there is an initial guess 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

After talking with Aayush, we found guesses are not propagated in a hierarchical system. Hopefully SciML/ModelingToolkit.jl#2654 should fix it

Copy link
Member Author

@sathvikbhagavan sathvikbhagavan Apr 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

One more bug from MTK - linearization_function doesn't use guesses - SciML/ModelingToolkit.jl#2656 should fix it

Copy link
Member

Choose a reason for hiding this comment

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

Linearization shouldn't necessarily use guesses. It should make sure the algebraic conditions are satisfied and that's it. That's a separate issue though, just update the test to be good and we can merge, the linearization failure is separate from this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean to mark it as broken/skip? done that in 9bc4b8b

Copy link
Member

Choose a reason for hiding this comment

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

no, fix it by passing in state values


# States:
- `u`: Value of the connector; if nin=1 this is a scalar
- `u`: Value of the connector which is a scalar.
""" RealInput

@connector function RealInputArray(; name, nin = 2, u_start = zeros(nin))
@variables u(t)[1:nin]=u_start [
input = true,
description = "Inner variable in RealInputArray $name"
]
u = collect(u)
ODESystem(Equation[], t, [u...], []; name = name)
end
@doc """
RealInputArray(;name, nin, u_start)

Connector with an array of input signals of type Real.

# Parameters:
- `nin=2`: Number of inputs.
- `u_start=zeros(nin)`: Initial value for `u`.

# States:
- `u`: Value of the connector which is an array.
""" RealInputArray

@connector function RealOutput(; name, nout = 1, u_start = nout > 1 ? zeros(nout) : 0.0)
nout > 1 && @warn "For outputs greater than one, use `RealOutputArray`."
if nout == 1
@variables u(t)=u_start [
output = true,
Expand All @@ -42,18 +64,38 @@ Connector with one input signal of type Real.
ODESystem(Equation[], t, [u...], []; name = name)
end
@doc """
RealOutput(;name, nout, u_start)
RealOutput(;name, u_start)

Connector with one output signal of type Real.

# Parameters:
- `nout=1`: Number of outputs
- `u_start=0`: Initial value for `u`
- `u_start=0`: Initial value for `u`.

# States:
- `u`: Value of the connector; if nout=1 this is a scalar
- `u`: Value of the connector which is a scalar.
""" RealOutput

@connector function RealOutputArray(; name, nout = 2, u_start = zeros(nout))
@variables u(t)[1:nout]=u_start [
output = true,
description = "Inner variable in RealOutput $name"
]
u = collect(u)
ODESystem(Equation[], t, [u...], []; name = name)
end
@doc """
RealOutputArray(;name, nout, u_start)

Connector with an array of output signals of type Real.

# Parameters:
- `nout=2`: Number of outputs.
Copy link
Member

Choose a reason for hiding this comment

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

That default makes no sense. Why 2?

Copy link
Member Author

Choose a reason for hiding this comment

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

As this was an array, I thought 2 element might be more common than 1 element for the default. I can change it to 1 if thats better.

Copy link
Member

Choose a reason for hiding this comment

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

Why would it have a default at all? This seems like it should be a required argument

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that would be better - addressed in 6bb905a

- `u_start=zeros(nout)`: Initial value for `u`.

# States:
- `u`: Value of the connector which is an array.
""" RealOutputArray

"""
SISO(;name, u_start = 0.0, y_start = 0.0)

Expand Down