Increase Accuracy of OpenQASM 2.0 Dump

[burgholzer]

Summary

This PR improves the accuracy of the OpenQASM 2.0 export by not truncating parameters of gates to single precision and using a tighter bound for checking whether a given parameter is close to a fraction of pi.
Resolves #7166.

Details and comments

For details, see the discussion in #7166. The additional precision is accomplished by relying on Python's built-in float formatting as a default in the pi_check function.

[qiskit-bot]

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the the following people are requested to review this:

• @Qiskit/terra-core

[CLAassistant]

All committers have signed the CLA.

[jakelishman]

Given the discussion in #7166, I'm fine with this. On the ndigits front: probably the ideal would be for us to have the default be None, and to use Python's default float formatting if ndigits is None (or the current implementation if not). In that mode, Python will output the shortest string which evaluates to the same target float - that's why str(0.3) is "0.3", str(0.3 * 3) == "0.8999999999999999" and str(0.9) == "0.9", for example.

If you'd like to do that in this PR, let me know, otherwise we can just move on to merge. Either way, please could you add an "upgrade" release note for this change (reno new --edit change-qasm-float-output) that explains that the number of digits output by QuantumCircuit.qasm() has changed to more accurately represent the parameters. If you want to make the above change, it'll be a "feature" note, rather than an "upgrade".

edit: the lint CI run is also failing, probably because there's a docstring line that's a bit too long in the new test you added. Nothing major - you can just split the line in two.

[burgholzer]

Thanks for your feedback and the detailed information.
I'll happily work out your suggestion in this PR. Just to double check that we are on the same page: what you are proposing is to change the default for the ndigits parameter in the pi_check method to None and rely on Python's built-in number printing in case no particular value is provided, right?

Of course I'll fix the linter warnings and create a corresponding release note in the process 👍🏼

[jakelishman]

Ok cool! Yeah, that's exactly it - if ndigits=None then we don't specify a format specifier (as opposed to :{ndigits}f).

[burgholzer]

I refactored the pi_check function accordingly and added corresponding release notes (which I hope are sufficiently accurate).

Some of the pi_check tests had to be adapted. The alternative there would have been to explicitly set ndigits=8 in the corresponding tests and leave everything there as is. Let me know if that should be changed.

One of the (potentially easy to fix) things in the pi_check function is the hard-coded check for zero in
https://github.com/Qiskit/qiskit-terra/blob/fe8ab43f1218d6997458fb3bf4011045f40932b0/qiskit/circuit/tools/pi_check.py#L69-L70
All other comparisons are conducted with respect to eps.
In my opinion, it would make the function more consistent, if this zero check was also conducted with respect to eps.

[jakelishman]

Yeah, your new form looks better to me, thanks, and all your changes look like they make a lot of sense. I really dislike the whole pi_check function as a whole, and I'm not surprised there's more hard-coded weirdness in it, to be honest... I agree that that line you've highlighted should also be a comparison to eps.

(I can't do a proper review on GitHub right now because it seems to have bugged out a little, and it's showing commits that aren't actually on the branch. Don't worry about it - your branch is actually fine, it's just GitHub being weird, and it'll fix itself in a bit, I'm sure.)

[nonhermitian]

I really dislike the whole pi_check function as a whole

That is a shame. It is actually quite useful for seeing when a given angle is near some commonly used fraction of pi. I agree it has been a bit mangled over time, but that is not its fault.

[burgholzer]

Yeah, your new form looks better to me, thanks, and all your changes look like they make a lot of sense. I really dislike the whole pi_check function as a whole, and I'm not surprised there's more hard-coded weirdness in it, to be honest... I agree that that line you've highlighted should also be a comparison to eps.

I updated the respective line to properly compare against eps and adjusted the tests to reflect the change.
I believe that the function itself has it's purpose but it also has the potential of introducing unnecessary approximations without people actively being aware of them.

(I can't do a proper review on GitHub right now because it seems to have bugged out a little, and it's showing commits that aren't actually on the branch. Don't worry about it - your branch is actually fine, it's just GitHub being weird, and it'll fix itself in a bit, I'm sure.)

Something seems to have gotten messed up in the git history. I rebased the commits and changes now show up properly again.
This should be ready for a proper review now.

[burgholzer]

@jakelishman gentle ping so this doesn't go stale again.

[jakelishman]

I read it quickly, and all the code looks sensible to me, thank you! I'm actually on paternity leave for the next few weeks, so it's difficult for me to check completely properly. @1ucian0, please could you take over and merge if you're happy?

[burgholzer]

@1ucian0 @jakelishman gentle ping. It's been another month 🙃

[jakelishman]

Sorry once more for this falling through the cracks. I've fixed the merge conflict that had arisen, and we can merge this for the 0.23 series.

[burgholzer]

No worries, thanks for getting back!

[coveralls]

Pull Request Test Coverage Report for Build 3499080974

• 10 of 11 (90.91%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.001%) to 84.583%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/circuit/tools/pi_check.py	10	11	90.91%

Totals
Change from base Build 3498826182:	0.001%
Covered Lines:	62524
Relevant Lines:	73920

---

💛 - Coveralls