Skip to content

Update gpindices shifts multiple times when you add in a new parameterized SPAM layer. #612

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

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

nkoskelo
Copy link

No description provided.

@nkoskelo nkoskelo requested review from rileyjmurray and a team as code owners July 22, 2025 22:49
@nkoskelo nkoskelo requested a review from coreyostrove July 22, 2025 22:49
Copy link
Contributor

@rileyjmurray rileyjmurray left a comment

Choose a reason for hiding this comment

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

Thanks for this fix, and for writing the tests!

I have two requests.

  1. Please make the tests instance methods of a new class, TestRebuildParamvec, that subclasses unittest.TestCase.
  2. Please update the PR opening comment to explain the purpose of the PR. You, me, and the other pyGSTi devs will know the purpose for the near future, but we'll inevitably forget.

I'll note that a search through the pyGSTi codebase shows that ModelMember.shift_gpindices is called only in two places: once recursively in shift_gpindices itself, and once in OpModel._rebuild_paramvec. The latter wasn't passing in any value for the "memo" argument in ModelMember.shift_gpindices.

@coreyostrove
Copy link
Contributor

We'll also need to track down what is going on with the failing unit tests. These look substantive, so there may be some unexpected consequences being caught. Do these fail locally on your system?

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