-
Notifications
You must be signed in to change notification settings - Fork 13
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
pass vectors along wires for instantaneous machines #112
pass vectors along wires for instantaneous machines #112
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Just a couple of small comments. Let me know if anything needs clarification!
@@ -144,7 +176,7 @@ The readout function `r` may depend on the state, parameters and time, so it mus | |||
If it is left out, then ``r=u``. | |||
""" | |||
const ContinuousMachine{T,I} = Machine{T, I, ContinuousDirectedSystem{T}} | |||
const InstantaneousContinuousMachine{T} = Machine{T, InstantaneousDirectedInterface{T}, ContinuousDirectedSystem{T}} | |||
const InstantaneousContinuousMachine{T,I} = Machine{T, I, ContinuousDirectedSystem{T}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check here that the given interface I
is a subtype of AbstractInstantaneousDirectedInterface{T}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@slibkind I tried to implement something like const InstantaneousContinuousMachine{T,I<: AbstractInstantaneousDirectedInterface{T}} = Machine{T, I, ContinuousDirectedSystem{T}}
but it ran into problems with the constructors as implemented. That is, the constructor typed as {T,N}
complained when N
was given as an Int64
for the dimensions of the vectors on the ports, because it expected an interface. So I did the checking in the constructors.
I think this is a general problem we have to live with because when I tried to do some type checking for regular continuous machines as const ContinuousMachine{T,I <: AbstractDirectedInterface{T}} = Machine{T, I, ContinuousDirectedSystem{T}}
the constructor typed as {T,N}
also failed with the type error ERROR: TypeError: in Type, in I, expected I<:AlgebraicDynamics.DWDDynam.AbstractDirectedInterface{Float64}, got a value of type Int64
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree that having the vector length as part of the type parameter has led to some clunky issues. I made note of this in Issue #113.
test/dwd_dynam.jl
Outdated
m = InstantaneousContinuousMachine{Float64,3}( | ||
1, 3, 1, | ||
(u, x, p, t) -> u*1.5, # dynamics | ||
(u, x, p, t) -> u+x, # readout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised this works, since u
is a vector of length 3 and x
is a vector (of length 1) of vectors of length 3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
Thanks for the review @slibkind! I updated my PR with some fixes. Please see my responses. |
Adds a new interface
InstantaneousDirectedVectorInterface
for instantaneous machines that want to pass vectors along wires. Currently only working for Delay and Continuous machines.