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

RFC: Layerwise Richardson Extrapolation #2329

Merged
merged 1 commit into from
May 24, 2024
Merged

RFC: Layerwise Richardson Extrapolation #2329

merged 1 commit into from
May 24, 2024

Conversation

purva-thakre
Copy link
Contributor

@purva-thakre purva-thakre commented Apr 26, 2024

Copy link

codecov bot commented Apr 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.19%. Comparing base (ccb9ff6) to head (bee9d7a).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2329   +/-   ##
=======================================
  Coverage   98.19%   98.19%           
=======================================
  Files          87       87           
  Lines        4052     4052           
=======================================
  Hits         3979     3979           
  Misses         73       73           

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

@purva-thakre
Copy link
Contributor Author

I'll be late to the Community call today due to another appointment right before.

Apologies for the delay in adding this RFC! We can discuss the proposed changes async through the Google doc comments if there's not enough time during the community call.

@natestemen
Copy link
Member

The plan is to leave this PR unmerged until the RFC is accepted. Anyone who would like to give comments on the RFC is welcome to during the next two (say?) weeks.

cc @unitaryfund/engineering

@purva-thakre
Copy link
Contributor Author

purva-thakre commented Apr 29, 2024

Andrea left a comment on the RFC that we should keep the LRE and ZNE inference methods separate. Initially, I thought it would be better to move mitiq.zne.inference to mitiq.inference in order to avoid defining another module for the multivariate extrapolation methods.

The refactor to the Factory class in mitiq would have comprised of : current functions work for single variable extrapolation and there would be another option for the multivariate extrapolation. Based on Andrea's comments, this refactor would be a major breaking change.

https://docs.google.com/document/d/1oFRl4wMGMtn57V0c_1egaHh0WUUAbtgW-U_QxNL9_kY/edit?disco=AAABMa8k_iI

FWIW I do agree with Andrea that this would be a major breaking change. I thought of proposing this breaking change to make it possible for us to extend multivariate extrapolation beyond Richardson extrapolation (in the near future).

I would appreciate comments from others on the two comments in the Backward Compatibility section. I can work on making changes to the RFC afterward.

Edit: Maybe, I am missing something here. I'll try to understand why a Factory refactor might not work for multivariate extrapolation and update the details in the RFC soon.

@jordandsullivan
Copy link
Contributor

Andrea left a comment on the RFC that we should keep the LRE and ZNE inference methods separate. Initially, I thought it would be better to move mitiq.zne.inference to mitiq.inference in order to avoid defining another module for the multivariate extrapolation methods.

I agree that it would make sense to move all the inference methods into their own module as you describe @purva-thakre .

@purva-thakre
Copy link
Contributor Author

Current version of the RFC (without the linked Jupyter notebook) was accepted during this week's community call.

Copy link
Contributor

@cosenal cosenal left a comment

Choose a reason for hiding this comment

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

Merging this, given that the RFC has been accepted.

@cosenal cosenal merged commit 5880803 into main May 24, 2024
18 checks passed
@cosenal cosenal deleted the lre_rfc branch May 24, 2024 14:38
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.

RFC: Layerwise Richardson Extrapolation (LRE)
4 participants