-
-
Notifications
You must be signed in to change notification settings - Fork 209
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
refactor: major cleanup of *Problem
construction
#3121
Conversation
34a8c8e
to
a41dca2
Compare
a41dca2
to
03e294f
Compare
Is there a reason |
No, I just missed it. Will include them as well. |
Thanks! |
Extensions failure is an Enzyme bug |
@@ -37,7 +37,7 @@ function generate_initializesystem(sys::ODESystem; | |||
# set dummy derivatives to default_dd_guess unless specified | |||
push!(defs, x[1] => get(guesses, x[1], default_dd_guess)) | |||
end | |||
for (y, x) in u0map | |||
function process_u0map_with_dummysubs(y, x) |
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 create a closure?
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 reduces code duplication, since array symbolics need to be treated as arrays of scalars below
- `allow_symbolic` allows the returned array to contain symbolic values. If this is `true`, | ||
`promotetoconcrete` is set to `false`. | ||
""" | ||
function better_varmap_to_vars(varmap::AbstractDict, vars::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.
why is this not just varmap to vars?
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.
Because varmap_to_vars
is public API, so I can't change it. This does less than what varmap_to_vars
does.
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.
deprecate the old varmap to vars?
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.
We need to have an equivalent for people to use, which we don't (at least, not a single function that does everything)
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 look into writing varmap_to_vars
in terms of the new infrastructure
Close #3044
Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
Additional context
Add any other context about the problem here.