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

drop scalarizing #1052

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

drop scalarizing #1052

wants to merge 14 commits into from

Conversation

isaacsas
Copy link
Member

@isaacsas isaacsas commented Sep 13, 2024

Closes #1051

Not handled yet

  • parameters are not being left as arrays/vectors when using the two-argument constructor
  • scalarizing in the DSL
  • scalarizing in spatial code I'm not sufficiently familiar with the spatial code, and tests are passing I think, so I will leave this for @TorkelE to handle when he has time. See scalarization in spatial code #1059

WIth this PR the DSL will still scalarize non-parameters, but I believe this is consistent with MTK now.

Comment on lines -133 to -137
u0 = [10.0, 11.0, 12.0, 13.0]
ps = [float(x) for x in 100:119]
prob = ODEProblem(rs, u0, (0, 10.0), ps)
@test [prob.ps[k[i]] for i in 1:20] == ps
@test prob.u0 == u0
Copy link
Member Author

Choose a reason for hiding this comment

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

These are not appropriate inputs since they aren't mappings, hence I removed them.

@isaacsas isaacsas changed the title [WIP] try dropping scalarizing drop scalarizing Sep 20, 2024
@@ -485,16 +485,17 @@ function make_ReactionSystem_internal(rxs_and_eqs::Vector, iv, us_in, ps_in;
spatial_ivs = nothing, continuous_events = [], discrete_events = [],
observed = [], kwargs...)

# Filters away any potential observables from `states` and `spcs`.
obs_vars = [obs_eq.lhs for obs_eq in observed]
us_in = filter(u -> !any(isequal(u, obs_var) for obs_var in obs_vars), us_in)
Copy link
Member Author

@isaacsas isaacsas Sep 20, 2024

Choose a reason for hiding this comment

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

@TorkelE I modified this as I think it makes more sense to disallow including observable variables in the unknowns, and to instead give users an error (rather than filtering them out). This also seems less likely to lead to user code bugs where a user thinks something is an unknown but it is implicitly an observable.

@isaacsas
Copy link
Member Author

@TorkelE if tests pass I am happy with this (I'd like to get a new release with this out). Can you let me know if it looks OK to you?

I guess the only other question is if this requires a breaking release? I think it may since we are changing how parameters are handled to drop scalarizing, which will break peoples existing codes unfortunately. If you are OK I'll make this V15 and add a note that removing scalarizing is the only change.

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.

Extemely slow creation of ODEProblem from ReactionSystem
1 participant