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

Trotter Costs #359

Merged
merged 41 commits into from
Oct 17, 2023
Merged

Trotter Costs #359

merged 41 commits into from
Oct 17, 2023

Conversation

fdmalone
Copy link
Collaborator

@fdmalone fdmalone commented Sep 8, 2023

I'm opening this as a draft at the moment for some feedback, but it may be good to merge and follow up with some issues later:

Issues:

  1. The QVR is the naive way which I think can be improved from n log (1/e) to log (1/e).
  2. We had some questions whether the potential was suboptimally implemented (c.f. the first quantized block encoding potential doesn't have this eta^2 cost?
  3. I started to go down a bit of a rabbit hole with fixed point arithmetic. I would like to revisit the interpolation section in the notebook to use fixed point and a proxy for QROM so that we check the quality of interpolation directly. Could be a nice demonstration of classical_vals
  4. I have a huge hack to render the bloq counts for QROM which I think @tanujkhattar had a better solution to.

I think 4 should at least be resolved before merging. But yeah probably good to get some feedback before it gets any bigger.

@fdmalone
Copy link
Collaborator Author

fdmalone commented Sep 8, 2023

I committed some things accidentally, need to rebase.

@fdmalone fdmalone marked this pull request as ready for review September 19, 2023 05:45
@fdmalone fdmalone requested a review from ncrubin September 22, 2023 00:43
@fdmalone
Copy link
Collaborator Author

@ncrubin I restructured a bit if you want to take a look

@fdmalone
Copy link
Collaborator Author

fdmalone commented Oct 9, 2023

@mpharrigan ready for review

Copy link
Collaborator

@mpharrigan mpharrigan left a comment

Choose a reason for hiding this comment

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

wow! Lots of nice work.

Some nits to address

qualtran/bloqs/chemistry/trotter.py Outdated Show resolved Hide resolved
qualtran/bloqs/chemistry/trotter.py Outdated Show resolved Hide resolved
qualtran/bloqs/chemistry/trotter.py Outdated Show resolved Hide resolved
qualtran/bloqs/chemistry/trotter.py Outdated Show resolved Hide resolved
qualtran/bloqs/chemistry/trotter.py Show resolved Hide resolved
qualtran/bloqs/chemistry/trotter.ipynb Outdated Show resolved Hide resolved
qualtran/bloqs/chemistry/trotter.ipynb Outdated Show resolved Hide resolved
qualtran/bloqs/chemistry/trotter.ipynb Outdated Show resolved Hide resolved
qualtran/bloqs/chemistry/trotter.ipynb Show resolved Hide resolved
qualtran/bloqs/chemistry/trotter.ipynb Show resolved Hide resolved
@fdmalone
Copy link
Collaborator Author

fdmalone commented Oct 13, 2023

@mpharrigan I addressed everything I think.

@fdmalone fdmalone merged commit adf7059 into main Oct 17, 2023
5 checks passed
@fdmalone fdmalone deleted the trotter branch October 17, 2023 04:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants