Skip to content

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

Merged
merged 7 commits into from
Mar 12, 2024

Conversation

MasonProtter
Copy link
Contributor

@MasonProtter MasonProtter commented Mar 10, 2024

Without this method, we hit the generic methods for MTKParameters which assumes ParameterIndex is an integer

Checklist

  • Appropriate tests were added
  • Any code changes were done in a way that does not break public API
  • All documentation related to code changes were updated
  • The new code follows the
    contributor guidelines, in particular the SciML Style Guide and
    COLPRAC.
  • Any new documentation only uses public API

Additional context

Copy link
Member

@AayushSabharwal AayushSabharwal left a 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].

@MasonProtter
Copy link
Contributor Author

I see, thank you. does this change I just pushed make more sense?

Copy link
Member

@AayushSabharwal AayushSabharwal left a 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.

Comment on lines 401 to 402
elseif portion === DEPENDENT_PORTION
setindexer(p.dependent)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
elseif portion === DEPENDENT_PORTION
setindexer(p.dependent)

The dependent portion shouldn't be set directly

Copy link
Contributor Author

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

Copy link
Member

@AayushSabharwal AayushSabharwal left a 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

@ChrisRackauckas
Copy link
Member

Needs formatting

@MasonProtter
Copy link
Contributor Author

I added a basic testset to try and cover all the branches. Let me know if that's appropriate.

@ChrisRackauckas
Copy link
Member

Format's last PR

@ChrisRackauckas ChrisRackauckas merged commit 7bf1d52 into SciML:master Mar 12, 2024
@MasonProtter MasonProtter deleted the patch-1 branch March 13, 2024 11:42
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.

3 participants