-
Notifications
You must be signed in to change notification settings - Fork 3
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
Feature/componentarray input output #21
base: main
Are you sure you want to change the base?
Feature/componentarray input output #21
Conversation
…imization.jl into feature/typed-wiring-diagrams
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.
Overall a very nice PR. I think there are some small specifics to clean up that I left comments on.
@@ -1,30 +1,17 @@ | |||
# Implement the cospan-algebra of dynamical systems. | |||
module Optimizers | |||
|
|||
export pullback_matrix, pushforward_matrix, Optimizer, OpenContinuousOpt, OpenDiscreteOpt, Euler, |
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 think we should still support the matrix versions of these functions.
src/Optimizers.jl
Outdated
export pullback_matrix, pushforward_matrix, Optimizer, OpenContinuousOpt, OpenDiscreteOpt, Euler, | ||
simulate | ||
export Optimizer, OpenContinuousOpt, OpenDiscreteOpt, Euler, | ||
simulate, pullback_function, pushforward_function, isapprox |
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.
It's bad practice to export functions from Base. We should just define the version of isapprox we need in the relevant test file (it's not critical to the function of this package).
|
||
|
||
""" pullback_function(f::FinFunction, v::Vector) |
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 think we want pullback and pushforward functions to return the functions that do the pushforward and pullback when applied to a vector (similar to the matrix versions). This is because usually we will be calling pushforward and pullback with the same finfunction but different vectors over and over. So it would be nice to generate the function for a single finfunction once and then be able to call that multiple times on different inputs. This also maintains consistency with the matrix version.
# Run a discrete optimizer the designated number of time-steps. | ||
function simulate(f::Open{Optimizer}, d::AbstractUWD, x0::ComponentArray, tsteps::Int) |
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 can't fight the feeling that there is a simpler/cleaner way to do this. I'm not sure what it is, but I will think about it.
test/Objectives.jl
Outdated
|
||
|
||
# Test ComponentArray version of input/output on scalar variables | ||
# Note that all variables must be exposed to use this i/o system |
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.
Why do all variables have to be exposed?
test/Objectives.jl
Outdated
dc2 = composite_of_optimizers | ||
|
||
|
||
x2 = ComponentArray(a=11.0, b=22, c=33, d=44, e=55, f=66, g=77, h=88, i=99) |
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.
Why is there an i component here when there is no i variable in the UWD?
This includes the changes from my other PR, plus: