-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Some unit tests crash when using symengine #12359
Comments
For some reason, if import symengine
from qiskit.circuit.parameter import Parameter
from qiskit.circuit.parameterexpression import ParameterExpression
theta = Parameter("theta")
phi = Parameter("phi")
sum_param = theta + phi
sum_param._symbol_expr.__reduce__() |
Thanks for the report. Do you know what version of symengine you're using? Fwiw, it would be really pretty weird if this is caused by Qiskit and not some allocator problem within Symengine - our use of it is very much inline with their public interface, and we don't go messing around in their internals or anything. |
I'm using symengine 0.11.2, and Arch Linux builds the library with the following options: cmake -B build -S $pkgname-$pkgver \
-DCMAKE_INSTALL_PREFIX=/usr \
-DBUILD_SHARED_LIBS=ON \
-DWITH_TCMALLOC=ON \
-DWITH_PTHREAD=ON \
-DWITH_SYMENGINE_THREAD_SAFE=ON \
-DWITH_ARB=ON \
-DWITH_ECM=ON \
-DWITH_LLVM=ON \
-DWITH_MPFR=ON \
-DWITH_MPC=ON \
-DWITH_PRIMESIEVE=ON \
-DWITH_BOOST=ON \
-DWITH_COTIRE=OFF \
-DWITH_SYSTEM_CEREAL=ON
cmake --build build |
If you installed The difference between pre-importing If you have the means to try, would you be able to see if the error repeats if you drop the |
I was going to do that experiment next, yes. I will rebuild I just installed |
I can confirm those tests don't fail when using |
Thanks so much for looking into it in depth! I guess if the issue is somewhere in Arch / tcmalloc / symengine, we can close this issue on Qiskit? I'll close it, but feel free to re-open if there's more to discuss. |
After solving the crashes due to In particular is this test:
It works running on a virtual env, but if fails using system shared libraries. It's a numerical error happening in this line: qiskit/qiskit/circuit/parameterexpression.py Line 534 in 235e581
Running on my system Lines 71 to 74 in 235e581
The reason is that Note also this relevant comment from NumPy (see Notes):
Perhaps it's better to use |
Sorry, I guess |
Also, after thinking a bit I don't understand why there should be such a rounding error in the first place. Do you know why is that error expected from that comment in the code and why |
No difference compiling |
I noticed that the
This causes frequency = self.disassemble_value(instruction.frequency) * 10**9 This is what ultimately leads to the numerical error that could have been avoided and has nothing to do with the double precision of the |
And here is a small self-contained example replicating the issue with pure import symengine
f = symengine.Symbol("f")
expr = f / 1000
expr_wrong = expr * 1e9
expr_good = expr * 10**9
wrong_value = float(expr_wrong.subs("f", 3.14))
good_value = float(expr_good.subs("f", 3.14))
print(wrong_value)
# 3139999.9999999995
print(good_value)
# 3140000.0 |
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 using symengine. Instead, this scaling can be done multiplying by integer 10**9. See: Qiskit#12359 (comment)
Sorry for the lack of response here - I reopened the issue, started typing a comment, then got distracted and forgot I hadn't posted. Thanks so much for tracking this down and providing the fix - that's a very tricksy bug to find, and thanks for filing it on |
No problem ;) |
* Avoid lossing precision when scaling frequencies 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 using symengine. Instead, this scaling can be done multiplying by integer 10**9. See: #12359 (comment) * Add release note --------- Co-authored-by: Jake Lishman <[email protected]>
* Avoid lossing precision when scaling frequencies 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 using symengine. Instead, this scaling can be done multiplying by integer 10**9. See: #12359 (comment) * Add release note --------- Co-authored-by: Jake Lishman <[email protected]> (cherry picked from commit 96607f6)
* Avoid lossing precision when scaling frequencies 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 using symengine. Instead, this scaling can be done multiplying by integer 10**9. See: #12359 (comment) * Add release note --------- Co-authored-by: Jake Lishman <[email protected]> (cherry picked from commit 96607f6) Co-authored-by: Iyán <[email protected]>
* Avoid lossing precision when scaling frequencies 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 using symengine. Instead, this scaling can be done multiplying by integer 10**9. See: #12359 (comment) * Add release note --------- Co-authored-by: Jake Lishman <[email protected]> (cherry picked from commit 96607f6) Co-authored-by: Iyán <[email protected]> (cherry picked from commit 0a05ea2)
* Avoid lossing precision when scaling frequencies 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 using symengine. Instead, this scaling can be done multiplying by integer 10**9. See: #12359 (comment) * Add release note --------- Co-authored-by: Jake Lishman <[email protected]> (cherry picked from commit 96607f6)
* Avoid lossing precision when scaling frequencies 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 using symengine. Instead, this scaling can be done multiplying by integer 10**9. See: Qiskit#12359 (comment) * Add release note --------- Co-authored-by: Jake Lishman <[email protected]>
* Avoid lossing precision when scaling frequencies 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 using symengine. Instead, this scaling can be done multiplying by integer 10**9. See: #12359 (comment) * Add release note --------- Co-authored-by: Jake Lishman <[email protected]> (cherry picked from commit 96607f6) Co-authored-by: Iyán <[email protected]> Co-authored-by: Luciano Bello <[email protected]>
Environment
What is happening?
Some unit tests fail due to symengine crashes. For example, the following five tests from
test_circuit_load_from_qpy.py
crash on my system:test_parameter_expression
test_evolutiongate_param_expr_time
test_parameter_expression_global_phase
test_parameter_vector_element_in_expression
test_symengine_full_path
From the first test, the exact line when the the test crashes is this:
qiskit/qiskit/qpy/binary_io/value.py
Line 58 in 6ff0eb5
Which produces a
src/tcmalloc.cc:304] Attempt to free invalid pointer 0x6146ecca5b60
.How can we reproduce the issue?
Here is a small self-contained script that can replicate the crash described above.
The following running on the same system works:
What should happen?
Ideally, all unit tests would pass in a clean chroot env on Arch Linux.
Any suggestions?
No response
The text was updated successfully, but these errors were encountered: