Avoid lossing precision when scaling frequencies

[iyanmv]

Summary

Classes in pulse_instruction.py scale frequency values to GHz by multipliying ParameterExpression with float 1e9. This can lead to numerical errors on some systems due to symengine "rounding" errors. Instead, this scaling can be done multiplying by integer 10**9.

Details and comments

In this unit test

qiskit/test/python/qobj/test_pulse_converter.py

Line 343 in 235e581

def test_parameterized_shift_frequency(self):

The frequency string "f / 1000" gets converted to ParameterExpression(1000000.0*f) after ParameterExpression(f/1000) is multiplied by 1e9. For some unknown reason, when the symbol f is later substituted with the value 3.14, and the RealDouble is converted to float, an error is introduced that can't be fixed by

qiskit/qiskit/pulse/utils.py

Lines 71 to 74 in 235e581

	# Remove truncation error and convert the result into Python builtin type.
	# Value could originally contain a rounding error, e.g. 1.00000000001
	# which may occur during the parameter expression evaluation.
	evaluated = np.round(operand, decimals=decimal).item()

This fixes: #12359 (comment)

Upstream issue: symengine/symengine.py#476

[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
• @nkanazawa1989

[coveralls]

Pull Request Test Coverage Report for Build 9100081845

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
• 35 unchanged lines in 6 files lost coverage.
• Overall coverage increased (+0.004%) to 89.635%

---

Files with Coverage Reduction	New Missed Lines	%
qiskit/transpiler/passes/synthesis/unitary_synthesis.py	2	88.2%
crates/qasm2/src/expr.rs	2	94.26%
crates/qasm2/src/lex.rs	4	92.62%
crates/qasm2/src/parse.rs	6	97.61%
qiskit/dagcircuit/dagnode.py	8	88.12%
crates/circuit/src/circuit_instruction.rs	13	87.59%

Totals
Change from base Build 9037095581:	0.004%
Covered Lines:	62311
Relevant Lines:	69516

---

💛 - Coveralls

[jakelishman]

Thanks so much for all the work to track this down! This seems like an easy change to make - in 99% of cases, Python's going to immediately convert the int to a float, but in the case that the pulse is symbolic, it's good not to introduce the floating-point error into the calculation.

I pushed up a release note, but otherwise this is good to go.

[1ucian0]

@Mergifyio backport stable/0.46

[mergify]

backport stable/0.46

✅ Backports have been created

• #12480 Avoid lossing precision when scaling frequencies (backport #12392) has been created for branch stable/0.46