Fix pulse parameter value formatter bug

[nkanazawa1989]

Summary

This PR fix the pulse parameter assignment bug that happens with input of particular integer number.

Fix #11971

Details and comments

[qiskit-bot]

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

• @Qiskit/terra-core
• @nkanazawa1989

[coveralls]

Pull Request Test Coverage Report for Build 8283966462

Details

• 15 of 15 (100.0%) changed or added relevant lines in 1 file are covered.
• 15 unchanged lines in 2 files lost coverage.
• Overall coverage increased (+0.01%) to 89.263%

---

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/lex.rs	3	91.94%
crates/qasm2/src/parse.rs	12	97.15%

Totals
Change from base Build 8282173373:	0.01%
Covered Lines:	59574
Relevant Lines:	66740

---

💛 - Coveralls

[wshanks]

This looks good. I had a few questions. I think the issue only comes into play in practice for very large integers (the case leading to #11971 was caused by numbers not being multiplied by dt (2e-10) so they were much larger than expected), so it is not too urgent.

[wshanks]

I would add a comment here that ints are being returned early to avoid rounding errors in the code below.

I was trying to understand what was wrong with the old code. np.round's documentation says that it effectively multiplies by 10**decimal (probably casting to float), rounds to an integer, and divides by 10**decimal. For a large enough integer, this operation must sometimes introduce rounding error in the float operations that causes is_integer in the old code to fail.

I was surprised because I was expecting your test cases to be around the 64 bit floating point precision of 1e19 but they are around 1e12. It kind of makes sense to me if there is an extra 1e10 in the operations.

[wshanks]

Do you think this is still important? In the old code, the number was always converted from int to float so this was needed to convert the input ints back into ints. With your changes we now know that the input was not an int so it might be okay to leave it as a float. Maybe some users rely on the casting. is_integer seems fragile combined with round though. Something that tests for almost an integer might be better.

[nkanazawa1989]

Yes. If the expression is something like 1.00000000001 (could happen if we numerically compute pulse duration), this object is the float type in early type check, and rounded to an integer and typecasted here. This is covered by the unit tests.

[nkanazawa1989]

Probably very large integer with tiny rounding error might be worth testing. A number with many significant digits is rounded anyways.

[wshanks]

Just to check -- do you think is_integer is good enough here or should it be

Suggested change

	if evaluated.is_integer():
	return int(evaluated)
	if np.isclose(evaluated % 1, 0):
	return int(evaluated)

to account for rounding error more leniently? Part of the original problem was that np.round did not always return something that would pass is_integer for an integer input.

One concern I have with this is that I think numpy.round could introduce floating point error that makes the number slightly below the nearest integer and then int(evaluated) could round down to the next integer. I guess it could be return int(evaluated + 0.1) to be safe though that is ugly.

[nkanazawa1989]

That's neat. Updated in 8ed6192. Thanks!

[wshanks]

I did some testing because I was still worried about rounding errors. What I found in practice was that is_integer always matched numpy.isclose(val % 1, 0), so numpy round must reliably round up above the nearest integer?

I undid your most recent change since is_integer seems clearer if there is no computational benefit to the modulus method.

Here is my script for reference. I found that casting to float, rounding, and casting back consistently matched the original number for numbers below 2**52, which makes sense since that is the precision of the significand in a 64 bit float. I got some surprising results for the fraction of results that did not match the input but that might be because my intuition was off for random numbers and exponential ranges.

import numpy


bad_counts = 0
min_bad = float("inf")
max_err = 0
max_nums = 100_000
total = 0
decimals = 1
max_int = 2**62
max_num = 0
for idx in range(10_000):
    start = numpy.random.randint(0, max_int, size=(10_000,))
    cast = start.astype(numpy.complex128)
    rounded = numpy.round(cast, decimals=decimals).astype(float)
    quotient, remainder = numpy.divmod(rounded, 1)
    # print(f"Round {idx}")
    int_checks = numpy.isclose(remainder, 0)
    # print(f"All close to ints still: {numpy.all(int_checks)}")
    final = quotient.astype(int)
    changed = start != final
    py_is_int = numpy.array([float(f).is_integer() for f in rounded])
    print(f"Not is_int:   {sum(numpy.logical_not(py_is_int))}")
    print(f"Not is_close: {sum(numpy.logical_not(int_checks))}")
    if not numpy.all(py_is_int == int_checks):
        print("is_integer differs from divmod")
    # print(f"int cast is equal: {numpy.all(changed)}")
    if numpy.all(int_checks) and numpy.any(changed):
        for start_i, final_i in zip(start[changed], final[changed]):

            pulse = int(numpy.real(numpy.round(complex(start_i), decimals=decimals)).item())
            # print("Bad input:")
            # print(f"Input:          {start_i}")
            # print(f"Output (numpy): {final_i}")
            # print(f"Output (pulse): {pulse}")
            if pulse != final_i:
                print(f"Pulse diff: {final_i}, {pulse}")
                break
        bad_counts += len(changed)
        min_bad = min(min_bad, min(start[changed]))
        max_err = max(max_err, max(abs(final[changed] - start[changed])))
    max_num = max(max_num, max(start))

    total += len(start)
    if total >= max_nums:
        break

print(f"Minimum bad input: {min_bad} ({min_bad:0.4g}), log2={numpy.log2(min_bad)}")
print(f"Maximum error: {max_err}")
print(f"Bad counts: {bad_counts} / {total}")
print(f"Maximum number processed: {max_num} ({max_num:0.4g}), log2={numpy.log2(max_num)}")

[wshanks]

Looks good!

[1ucian0]

@Mergifyio backport stable/0.46

[mergify]

backport stable/0.46

✅ Backports have been created

• #12478 Fix pulse parameter value formatter bug (backport #11972) has been created for branch stable/0.46 but encountered conflicts