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

introduce cache array to make discrete-time state update atomic #2702

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

Conversation

baggepinnen
Copy link
Contributor

@baggepinnen baggepinnen commented May 6, 2024

One step towards solving #2701

system in issue above now produces the correct answer

image

@baggepinnen
Copy link
Contributor Author

@AayushSabharwal the test failure indicates that the indices used to index into p are of some magical type? Tests pass locally so I assume this is some recent change upstream?

@@ -326,6 +326,7 @@ function generate_discrete_affect(
# @show disc_range
affect! = :(function (integrator, saved_values)
@unpack u, p, t = integrator
cache = p[$disc_range] # Cache needed for atomic state update
Copy link
Member

Choose a reason for hiding this comment

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

Nope this is all in MTK. ParameterIndex is an index into MTKParameters. disc_range isa Vector{<:ParameterIndex}. I didn't notice the error, but we don't have getindex(::MTKParameters, ::Vector{<:ParameterIndex}) defined, so this call is invalid. I'm not sure how it passes locally.

I guess we either need a method for the above, which just broadcasts the getindex or do it manually here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's the difference between these two then? The view version is used in a lot of places

cache = p[$disc_range]
disc_unknowns = view(p, $disc_range)

Copy link
Member

Choose a reason for hiding this comment

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

That's interesting. I'll look into why view works, but I suspect it is because view internally just relies on getindex working for scalar indices.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, I see the difference now, I'll push a fix

@AayushSabharwal
Copy link
Member

So view won't work either. There are a bunch of branches based on whether has_index_cache(sys) && get_index_cache(sys) !== nothing (which is basically a check for if the parameter object is an array or MTKParameters). The view version is never used for MTKParameters.

@baggepinnen
Copy link
Contributor Author

There are likely additional compiler-related problems, outlined in #2701

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.

2 participants