Skip to content
This repository has been archived by the owner on Jan 12, 2024. It is now read-only.

Fix #206 and improve Trotter–Suzuki code quality. #207

Merged
merged 2 commits into from
Jan 22, 2020

Conversation

cgranade
Copy link
Contributor

This PR fixes #206 by adding in an appropriate /// # Remark noting the 𝑘–2𝑘 dichotomy. This PR also improves code quality in the Trotter–Suzuki decomposition feature more generally and renames DecomposeIntoTimeStepsCADecomposedIntoTimeStepsCA to reflect that this feature is a exposed as a function that returns a new evolution operation.

Copy link
Contributor

@guanghaolow guanghaolow 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 cleaning up the code + documentation Chris. I've added a correction on where the indexing convention differs.

/// [quant-ph/0508139](https://arxiv.org/abs/quant-ph/0508139). In
/// particular, `DecomposedIntoTimeStepsCA(_, 4)` corresponds to the
/// propagator $S_2(\lambda)$ in quant-ph/0508139.
///
Copy link
Contributor

Choose a reason for hiding this comment

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

DecomposedIntoTimeStepsCA(_, 4) actually corresponds to $S_4(\lambda)$ in quant-ph/0508139.

The difference in indexing occurs in the definition of _TrotterStepSize(order: Int).

Whereas we set let stepSizeOuter = _TrotterStepSize(order); in the implementation of _TrotterArbitraryImplCA<'T>, they define the function _AlternateStepSize(order: Int) such that our stepSizeOuter is equal to _AlternateStepSize(order/2).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, my apologies for misunderstanding where the index convention differed. I'll fix that, then.

@cgranade
Copy link
Contributor Author

@guanghaolow: Thanks for double-checking and for catching the indexing error. I've expanded a lot to help prevent that confusion in the future, if you could please take another quick look. Thanks!

@cgranade
Copy link
Contributor Author

@guanghaolow: Would you be willing to take a look at this again? Thanks for the help!

Copy link
Contributor

@guanghaolow guanghaolow left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@cgranade cgranade merged commit ffb4749 into master Jan 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

API documentation for DecomposeIntoTimeStepsCA is confusing.
2 participants