quantumlib / Qualtran

Qᴜᴀʟᴛʀᴀɴ is a Python library for expressing and analyzing Fault Tolerant Quantum algorithms.
https://qualtran.readthedocs.io/en/latest/
Apache License 2.0
132 stars 35 forks source link

update b coefficient for azure cost model #1051

Closed bdg221 closed 2 weeks ago

bdg221 commented 1 month ago

Fixes #1050

This PR updates the documentation for the Azure Cost Model to reflect the change in the b coefficient for the rotation approximation model. The new value matches the updated Azure QRE code.

google-cla[bot] commented 1 month ago

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

mpharrigan commented 3 weeks ago

It's important to be able to trace all these constants that are floating around. I've fired up https://arxiv.org/abs/2211.07629 and they still have the 5.3 value. Please add some text that explains that the beverland paper uses the wrong value and provide a citation for the "correct" value.

mpharrigan commented 3 weeks ago

but thanks for the contribution! you'll also need to sign the cla for us to merge this :)

bdg221 commented 3 weeks ago

HI @mpharrigan, the problem is the when you look at both the a and b coefficients. If you look at the issue that the PR fixes, it matches an issue and PR in Microsoft's code (and paper), as well. https://github.com/microsoft/qsharp/issues/1218

The problem is that while there is an a coefficient that is 0.53, but it has the corresponding b coefficient of 4.86. There is also an a coefficient of 0.56 with a b coefficient of 5.3.

The paper you referenced says:

image

Reference 78 is https://arxiv.org/html/2203.10064v2/#S2 Here is a copy of that table highlighting the two lines. (Again mentioned in the QSharp issue that I opened and has been since corrected.) image

If you check the Microsoft issue and PR, the correction changes the b coefficient to 4.86

image

Also, I have signed the CLA (though after the failed invocation.)

image
mpharrigan commented 3 weeks ago

I don't doubt that the new value is the correct one; I just think it would be helpful to note that it is wrong in the beverland paper. Otherwise: readers will see a sentence that says "they use 4.86 in the paper" but if they read the paper they will see a different number

NoureldinYosri commented 3 weeks ago

@mpharrigan @bdg221 I did notice the difference between the number in the Beverland paper and in https://arxiv.org/html/2203.10064v2/#S2 when I implemented this and that is why I refer to "the paper" (i.e. the Beverland paper) and also why the rotation model is called BeverlandEtAlRotationCost

https://github.com/quantumlib/Qualtran/blob/01ff5433c2147e980cb53ad7a7ab61c43668d961/qualtran/surface_code/rotation_cost_model.py#L96-L104

These coeficients are not actually constant but are parameters of the hardware and are computed by fitting models so they are expected to change over time as hardware gets better. The notebook for the Azure model should retain the value from the Beverland paper (perhabs with a note that these values change with the hardware).

You can always create an rotation model that uses the most up to date values RotationLogarithmicModel(slope=new_slope, overhead=new_overhead)

mpharrigan commented 3 weeks ago

This case is tricky because we'd want to match the numbers used in the azure resource estimator, which has indeed been updated (see linked prs). Maybe two 'models' is the solution, but it seems like a lot of effort to preserve an "incorrect" value

NoureldinYosri commented 3 weeks ago

in that case instead of exposing the BeverlandEtAlRotationCost we will should expose a AzureRotationModel object instead which should track the values from the Azure resource estimator, but he rotation model in the notebook should use the values from the Beverland paper in order to match with its estimates

msoeken commented 3 weeks ago

I think one model would be better that uses the correct values. Regarding the wording in the text, maybe an option could be to write something like: "In the paper they are using the mixed fallback rotation synthesis model for the Clifford+T gate set (whose coefficients are ...)".

mpharrigan commented 3 weeks ago

great, I'll defer to you @msoeken and @bdg221 to update the wording in the text to whatever you think is best and we can use the new number

NoureldinYosri commented 3 weeks ago

@msoeken I agree with this wording ... my only concern is the potential discrepancy between the estimates obtained using the new value and the estimates from the Beverland paper. Does the new value of $b$ change the results to the point that they disagree with the Beverland paper? if they don't then we can change the value in the object ... but if they do then the notebook should create its own rotation model object because the point of the notebook is reproducing the Beverland paper.

mpharrigan commented 3 weeks ago

A brief sentence in the notebook should be enough to explain any discrepancy