-
-
Notifications
You must be signed in to change notification settings - Fork 224
Add getindex
/setindex!
methods for MTKParameters
with ParameterIndex
#2531
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
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.
pind.idx
is not necessarily a 2-tuple. It can also have extra indices in case p.portion[i][j]
is an array. For example, for @parameters p[1:3]
you might have a ParameterIndex(Tunable(), (2, 1, 2))
to refer to p[2]
.
I see, thank you. does this change I just pushed make more sense? |
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 forgot this last time around 😅 but I think setindex!
should just call set_parameter!
. It handles the extra indexes, doesn't allow setting the dependent portion (as intended) and updates the dependent values.
src/systems/parameter_buffer.jl
Outdated
elseif portion === DEPENDENT_PORTION | ||
setindexer(p.dependent) |
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.
elseif portion === DEPENDENT_PORTION | |
setindexer(p.dependent) |
The dependent portion shouldn't be set directly
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.
Okay, I'd switched it to just fall back to set_parameter
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.
LGTM, but I don't have permissions to make CI run.
@ChrisRackauckas could you please approve CI
Needs formatting |
I added a basic testset to try and cover all the branches. Let me know if that's appropriate. |
Format's last PR |
Without this method, we hit the generic methods for
MTKParameters
which assumesParameterIndex
is an integerChecklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
Additional context