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

Pass keyword arguments of MomentumQNGOptimizer to base class; Fix QNGOptimizer update with singular metric tensor #6471

Merged
merged 10 commits into from
Oct 30, 2024

Conversation

astralcai
Copy link
Contributor

@astralcai astralcai commented Oct 29, 2024

Context:
The newly added MomentumQNGOptimizer in #6240 does not use the keyword arguments approx and lam.

Description of the Change:
Pass the unused arguments to super().__init__

Possible Drawbacks:
No tests are added for this, but these two parameters are inherited from the base class, and they are never tested in the existing test module for the base class to begin with.

Copy link
Contributor

Hello. You may have forgotten to update the changelog!
Please edit doc/releases/changelog-dev.md with:

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

Copy link

codecov bot commented Oct 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (v0.39.0-rc0@41a3b8a). Learn more about missing BASE report.

Additional details and impacted files
@@              Coverage Diff               @@
##             v0.39.0-rc0    #6471   +/-   ##
==============================================
  Coverage               ?   99.38%           
==============================================
  Files                  ?      452           
  Lines                  ?    42797           
  Branches               ?        0           
==============================================
  Hits                   ?    42535           
  Misses                 ?      262           
  Partials               ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@PietropaoloFrisoni PietropaoloFrisoni 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 the catch!

Copy link
Contributor

@dwierichs dwierichs left a comment

Choose a reason for hiding this comment

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

By now most of the changes were made by me, which makes the review a bit pointless. Maybe we can request one more review if desired.

@dwierichs dwierichs changed the title Pass unused argument to super init Pass keyword arguments of MomentumQNGOptimizer to base class; Fix QNGOptimizer update with singular metric tensor Oct 30, 2024
@dwierichs dwierichs added this to the v0.39 milestone Oct 30, 2024
@astralcai
Copy link
Contributor Author

By now most of the changes were made by me, which makes the review a bit pointless. Maybe we can request one more review if desired.

Thank you so much! @dwierichs

@JerryChen97
Copy link
Contributor

I can take a look if it's more like a linalg related issue

@JerryChen97 JerryChen97 self-requested a review October 30, 2024 13:33
@JerryChen97
Copy link
Contributor

In qml.optimize.qnspsa there's a similar usage of solve. Not sure yet if it should be treated as well @dwierichs @astralcai

Copy link
Contributor

@JerryChen97 JerryChen97 left a comment

Choose a reason for hiding this comment

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

LGTM! I already committed the same correction in momentum_qng which is the only thing I saw that needs to add

@astralcai astralcai merged commit 76ca29e into v0.39.0-rc0 Oct 30, 2024
39 checks passed
@astralcai astralcai deleted the qng-fix branch October 30, 2024 17:46
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.

4 participants