From 52e748a321307cad84969d6932c09fce1fcc7321 Mon Sep 17 00:00:00 2001 From: mhucka Date: Tue, 10 Dec 2024 05:24:44 +0000 Subject: [PATCH 1/7] Fix compiler warnings about unused variables This file produces many warnings of the form ``` tensorflow_quantum/core/src/circuit_parser_qsim.cc:185:8: warning: variable 'unused' set but not used [-Wunused-but-set-variable] 185 | bool unused = absl::SimpleAtoi(op.qubits(0).id(), &q0); | ^ ``` Inspecting the code reveals that variable `unused` is indeed set but never used. I replaced the code with simple testing of the return values, similar to what is used in one place in the file, and in a way that follows the recommendations from the Abseil docs. --- .../core/src/circuit_parser_qsim.cc | 126 +++++++++++++----- 1 file changed, 94 insertions(+), 32 deletions(-) diff --git a/tensorflow_quantum/core/src/circuit_parser_qsim.cc b/tensorflow_quantum/core/src/circuit_parser_qsim.cc index 2b3e81d19..e2a6a1346 100644 --- a/tensorflow_quantum/core/src/circuit_parser_qsim.cc +++ b/tensorflow_quantum/core/src/circuit_parser_qsim.cc @@ -157,7 +157,12 @@ inline Status SingleConstantGate( const unsigned int num_qubits, const unsigned int time, QsimCircuit* circuit, std::vector* metadata) { unsigned int q0; - (void)absl::SimpleAtoi(op.qubits(0).id(), &q0); + bool valid = absl::SimpleAtoi(op.qubits(0).id(), &q0); + if (!valid) { + return Status(absl::StatusCode::kInvalidArgument, + "Could not parse id of qubit 0"); + } + auto gate = create_f(time, num_qubits - q0 - 1); Status s = OptionalInsertControls(op, num_qubits, &gate); if (!s.ok()) { @@ -182,8 +187,17 @@ inline Status TwoConstantGate( const unsigned int num_qubits, const unsigned int time, QsimCircuit* circuit, std::vector* metadata) { unsigned int q0, q1; - bool unused = absl::SimpleAtoi(op.qubits(0).id(), &q0); - unused = absl::SimpleAtoi(op.qubits(1).id(), &q1); + bool valid = absl::SimpleAtoi(op.qubits(0).id(), &q0); + if (!valid) { + return Status(absl::StatusCode::kInvalidArgument, + "Could not parse id of qubit 0"); + } + valid = absl::SimpleAtoi(op.qubits(1).id(), &q1); + if (!valid) { + return Status(absl::StatusCode::kInvalidArgument, + "Could not parse id of qubit 1"); + } + auto gate = create_f(time, num_qubits - q0 - 1, num_qubits - q1 - 1); Status s = OptionalInsertControls(op, num_qubits, &gate); if (!s.ok()) { @@ -208,10 +222,13 @@ inline Status SingleEigenGate( const unsigned int num_qubits, const unsigned int time, QsimCircuit* circuit, std::vector* metadata) { unsigned int q0; - bool unused; float exp, exp_s, gs; Status u; - unused = absl::SimpleAtoi(op.qubits(0).id(), &q0); + bool valid = absl::SimpleAtoi(op.qubits(0).id(), &q0); + if (!valid) { + return Status(absl::StatusCode::kInvalidArgument, + "Could not parse id of qubit 0"); + } absl::optional exponent_symbol; u = ParseProtoArg(op, "exponent", param_map, &exp, &exponent_symbol); @@ -258,10 +275,17 @@ inline Status TwoEigenGate( QsimCircuit* circuit, std::vector* metadata) { unsigned int q0, q1; float exp, exp_s, gs; - bool unused; Status u; - unused = absl::SimpleAtoi(op.qubits(0).id(), &q0); - unused = absl::SimpleAtoi(op.qubits(1).id(), &q1); + bool valid = absl::SimpleAtoi(op.qubits(0).id(), &q0); + if (!valid) { + return Status(absl::StatusCode::kInvalidArgument, + "Could not parse id of qubit 0"); + } + valid = absl::SimpleAtoi(op.qubits(1).id(), &q1); + if (!valid) { + return Status(absl::StatusCode::kInvalidArgument, + "Could not parse id of qubit 1"); + } absl::optional exponent_symbol; u = ParseProtoArg(op, "exponent", param_map, &exp, &exponent_symbol); @@ -397,10 +421,13 @@ inline Status PhasedXGate(const Operation& op, const SymbolMap& param_map, const unsigned int time, QsimCircuit* circuit, std::vector* metadata) { int q0; - bool unused; float pexp, pexp_s, exp, exp_s, gs; Status u; - unused = absl::SimpleAtoi(op.qubits(0).id(), &q0); + bool valid = absl::SimpleAtoi(op.qubits(0).id(), &q0); + if (!valid) { + return Status(absl::StatusCode::kInvalidArgument, + "Could not parse id of qubit 0"); + } absl::optional exponent_symbol; u = ParseProtoArg(op, "exponent", param_map, &exp, &exponent_symbol); @@ -457,11 +484,18 @@ inline Status FsimGate(const Operation& op, const SymbolMap& param_map, QsimCircuit* circuit, std::vector* metadata) { int q0, q1; - bool unused; float theta, theta_s, phi, phi_s; Status u; - unused = absl::SimpleAtoi(op.qubits(0).id(), &q0); - unused = absl::SimpleAtoi(op.qubits(1).id(), &q1); + bool valid = absl::SimpleAtoi(op.qubits(0).id(), &q0); + if (!valid) { + return Status(absl::StatusCode::kInvalidArgument, + "Could not parse id of qubit 0"); + } + valid = absl::SimpleAtoi(op.qubits(1).id(), &q1); + if (!valid) { + return Status(absl::StatusCode::kInvalidArgument, + "Could not parse id of qubit 1"); + } absl::optional theta_symbol; u = ParseProtoArg(op, "theta", param_map, &theta, &theta_symbol); @@ -514,11 +548,18 @@ inline Status PhasedISwapGate(const Operation& op, const SymbolMap& param_map, const unsigned int time, QsimCircuit* circuit, std::vector* metadata) { int q0, q1; - bool unused; float pexp, pexp_s, exp, exp_s; Status u; - unused = absl::SimpleAtoi(op.qubits(0).id(), &q0); - unused = absl::SimpleAtoi(op.qubits(1).id(), &q1); + bool valid = absl::SimpleAtoi(op.qubits(0).id(), &q0); + if (!valid) { + return Status(absl::StatusCode::kInvalidArgument, + "Could not parse id of qubit 0"); + } + valid = absl::SimpleAtoi(op.qubits(1).id(), &q1); + if (!valid) { + return Status(absl::StatusCode::kInvalidArgument, + "Could not parse id of qubit 1"); + } absl::optional exponent_symbol; u = ParseProtoArg(op, "exponent", param_map, &exp, &exponent_symbol); @@ -605,10 +646,13 @@ inline Status AsymmetricDepolarizingChannel(const Operation& op, const unsigned int time, NoisyQsimCircuit* ncircuit) { int q; - bool unused; float p_x, p_y, p_z; Status u; - unused = absl::SimpleAtoi(op.qubits(0).id(), &q); + bool valid = absl::SimpleAtoi(op.qubits(0).id(), &q); + if (!valid) { + return Status(absl::StatusCode::kInvalidArgument, + "Could not parse id of qubit 0"); + } u = ParseProtoArg(op, "p_x", {}, &p_x); u = ParseProtoArg(op, "p_y", {}, &p_y); @@ -627,10 +671,13 @@ inline Status DepolarizingChannel(const Operation& op, const unsigned int time, NoisyQsimCircuit* ncircuit) { int q; - bool unused; float p; Status u; - unused = absl::SimpleAtoi(op.qubits(0).id(), &q); + bool valid = absl::SimpleAtoi(op.qubits(0).id(), &q); + if (!valid) { + return Status(absl::StatusCode::kInvalidArgument, + "Could not parse id of qubit 0"); + } u = ParseProtoArg(op, "p", {}, &p); if (!u.ok()) { @@ -645,10 +692,12 @@ inline Status DepolarizingChannel(const Operation& op, inline Status GADChannel(const Operation& op, const unsigned int num_qubits, const unsigned int time, NoisyQsimCircuit* ncircuit) { int q; - bool unused; float p, gamma; Status u; - unused = absl::SimpleAtoi(op.qubits(0).id(), &q); + if (!absl::SimpleAtoi(op.qubits(0).id(), &q)) { + return Status(absl::StatusCode::kInvalidArgument, + "Could not parse id of qubit 0"); + } u = ParseProtoArg(op, "p", {}, &p); if (!u.ok()) { @@ -669,8 +718,10 @@ inline Status ResetChannel(const Operation& op, const unsigned int num_qubits, const unsigned int time, NoisyQsimCircuit* ncircuit) { int q; - bool unused; - unused = absl::SimpleAtoi(op.qubits(0).id(), &q); + if (!absl::SimpleAtoi(op.qubits(0).id(), &q)) { + return Status(absl::StatusCode::kInvalidArgument, + "Could not parse id of qubit 0"); + } auto chan = qsim::Cirq::ResetChannel::Create(time, num_qubits - q - 1); ncircuit->channels.push_back(chan); @@ -682,10 +733,13 @@ inline Status AmplitudeDampingChannel(const Operation& op, const unsigned int time, NoisyQsimCircuit* ncircuit) { int q; - bool unused; float gamma; Status u; - unused = absl::SimpleAtoi(op.qubits(0).id(), &q); + bool valid = absl::SimpleAtoi(op.qubits(0).id(), &q); + if (!valid) { + return Status(absl::StatusCode::kInvalidArgument, + "Could not parse id of qubit 0"); + } u = ParseProtoArg(op, "gamma", {}, &gamma); if (!u.ok()) { @@ -702,10 +756,12 @@ inline Status PhaseDampingChannel(const Operation& op, const unsigned int time, NoisyQsimCircuit* ncircuit) { int q; - bool unused; float gamma; Status u; - unused = absl::SimpleAtoi(op.qubits(0).id(), &q); + if (!absl::SimpleAtoi(op.qubits(0).id(), &q)) { + return Status(absl::StatusCode::kInvalidArgument, + "Could not parse id of qubit 0"); + } u = ParseProtoArg(op, "gamma", {}, &gamma); if (!u.ok()) { @@ -723,10 +779,13 @@ inline Status PhaseFlipChannel(const Operation& op, const unsigned int time, NoisyQsimCircuit* ncircuit) { int q; - bool unused; float p; Status u; - unused = absl::SimpleAtoi(op.qubits(0).id(), &q); + bool valid = absl::SimpleAtoi(op.qubits(0).id(), &q); + if (!valid) { + return Status(absl::StatusCode::kInvalidArgument, + "Could not parse id of qubit 0"); + } u = ParseProtoArg(op, "p", {}, &p); if (!u.ok()) { @@ -743,10 +802,13 @@ inline Status BitFlipChannel(const Operation& op, const unsigned int num_qubits, const unsigned int time, NoisyQsimCircuit* ncircuit) { int q; - bool unused; float p; Status u; - unused = absl::SimpleAtoi(op.qubits(0).id(), &q); + bool valid = absl::SimpleAtoi(op.qubits(0).id(), &q); + if (!valid) { + return Status(absl::StatusCode::kInvalidArgument, + "Could not parse id of qubit 0"); + } u = ParseProtoArg(op, "p", {}, &p); if (!u.ok()) { From 4853328f7114a3c9173e1e004e9b4d850b518a93 Mon Sep 17 00:00:00 2001 From: mhucka Date: Tue, 10 Dec 2024 05:24:57 +0000 Subject: [PATCH 2/7] Add test cases for changes to circuit_parser_qsim.cc --- .../core/src/circuit_parser_qsim_test.cc | 66 +++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/tensorflow_quantum/core/src/circuit_parser_qsim_test.cc b/tensorflow_quantum/core/src/circuit_parser_qsim_test.cc index e4d487a85..62e82b146 100644 --- a/tensorflow_quantum/core/src/circuit_parser_qsim_test.cc +++ b/tensorflow_quantum/core/src/circuit_parser_qsim_test.cc @@ -480,6 +480,38 @@ TEST(QsimCircuitParserTest, SingleConstantGate) { } } +TEST(QsimCircuitParserTest, SingleConstantGateBadQubitId) { + Program program_proto; + Circuit* circuit_proto = program_proto.mutable_circuit(); + circuit_proto->set_scheduling_strategy(circuit_proto->MOMENT_BY_MOMENT); + Moment* moments_proto = circuit_proto->add_moments(); + + // Add gate. + Operation* operations_proto = moments_proto->add_operations(); + Gate* gate_proto = operations_proto->mutable_gate(); + gate_proto->set_id("I"); + + // Set the control args to empty. + google::protobuf::Map* args_proto = + operations_proto->mutable_args(); + (*args_proto)["control_qubits"] = MakeControlArg(""); + (*args_proto)["control_values"] = MakeControlArg(""); + + // Set the qubits. + Qubit* qubits_proto = operations_proto->add_qubits(); + qubits_proto->set_id("zero"); + + QsimCircuit test_circuit; + std::vector> fused_circuit; + SymbolMap empty_map; + std::vector metadata; + + ASSERT_EQ(QsimCircuitFromProgram(program_proto, empty_map, 1, &test_circuit, + &fused_circuit, &metadata), + tensorflow::Status(absl::StatusCode::kInvalidArgument, + "Could not parse id of qubit 0")); +} + TEST(QsimCircuitParserTest, SingleConstantGateControlled) { absl::flat_hash_map reference = { {"I", qsim::Cirq::I1::Create(0, 0).ControlledBy({1, 2}, {0, 0})}}; @@ -562,6 +594,40 @@ TEST(QsimCircuitParserTest, TwoConstantGate) { } } +TEST(QsimCircuitParserTest, TwoConstantGateBadQubitId) { + Program program_proto; + Circuit* circuit_proto = program_proto.mutable_circuit(); + circuit_proto->set_scheduling_strategy(circuit_proto->MOMENT_BY_MOMENT); + Moment* moments_proto = circuit_proto->add_moments(); + + // Add gate. + Operation* operations_proto = moments_proto->add_operations(); + Gate* gate_proto = operations_proto->mutable_gate(); + gate_proto->set_id("I2"); + + // Set the control args to empty. + google::protobuf::Map* args_proto = + operations_proto->mutable_args(); + (*args_proto)["control_qubits"] = MakeControlArg(""); + (*args_proto)["control_values"] = MakeControlArg(""); + + QsimCircuit test_circuit; + std::vector> fused_circuit; + SymbolMap empty_map; + std::vector metadata; + + // Set the qubit id's, but use a bad value for the 2nd one. + Qubit* qubits_proto = operations_proto->add_qubits(); + qubits_proto->set_id("0"); + qubits_proto = operations_proto->add_qubits(); + qubits_proto->set_id("one"); + + ASSERT_EQ(QsimCircuitFromProgram(program_proto, empty_map, 2, &test_circuit, + &fused_circuit, &metadata), + tensorflow::Status(absl::StatusCode::kInvalidArgument, + "Could not parse id of qubit 1")); +} + TEST(QsimCircuitParserTest, TwoConstantGateControlled) { absl::flat_hash_map reference = { {"I2", From 79682acf0d8dca72ebff8e09459307db52632f37 Mon Sep 17 00:00:00 2001 From: mhucka Date: Sat, 4 Jan 2025 22:07:05 +0000 Subject: [PATCH 3/7] Fix warning about int versus unsigned int --- tensorflow_quantum/core/src/circuit_parser_qsim_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tensorflow_quantum/core/src/circuit_parser_qsim_test.cc b/tensorflow_quantum/core/src/circuit_parser_qsim_test.cc index 62e82b146..7de46aa0a 100644 --- a/tensorflow_quantum/core/src/circuit_parser_qsim_test.cc +++ b/tensorflow_quantum/core/src/circuit_parser_qsim_test.cc @@ -64,7 +64,7 @@ Arg MakeControlArg(const std::string& val) { } inline void AssertControlEqual(const QsimGate& a, const QsimGate& b) { - for (int i = 0; i < a.controlled_by.size(); i++) { + for (unsigned int i = 0; i < a.controlled_by.size(); i++) { ASSERT_EQ(a.controlled_by[i], b.controlled_by[i]); } ASSERT_EQ(a.cmask, b.cmask); @@ -89,7 +89,7 @@ inline void AssertOneQubitEqual(const QsimGate& a, const QsimGate& b) { inline void AssertChannelEqual(const QsimChannel& a, const QsimChannel& b) { ASSERT_EQ(a.size(), b.size()); - for (int i = 0; i < a.size(); i++) { + for (unsigned int i = 0; i < a.size(); i++) { ASSERT_EQ(a[i].kind, b[i].kind); ASSERT_EQ(a[i].unitary, b[i].unitary); ASSERT_NEAR(a[i].prob, b[i].prob, 1e-5); From 0f0650f0017cddec1ac9adb7d36aa1634b4eb1bd Mon Sep 17 00:00:00 2001 From: mhucka Date: Fri, 10 Jan 2025 19:20:34 +0000 Subject: [PATCH 4/7] Revert "Fix compiler warnings about unused variables" This reverts commit 52e748a321307cad84969d6932c09fce1fcc7321. --- .../core/src/circuit_parser_qsim.cc | 126 +++++------------- 1 file changed, 32 insertions(+), 94 deletions(-) diff --git a/tensorflow_quantum/core/src/circuit_parser_qsim.cc b/tensorflow_quantum/core/src/circuit_parser_qsim.cc index e2a6a1346..2b3e81d19 100644 --- a/tensorflow_quantum/core/src/circuit_parser_qsim.cc +++ b/tensorflow_quantum/core/src/circuit_parser_qsim.cc @@ -157,12 +157,7 @@ inline Status SingleConstantGate( const unsigned int num_qubits, const unsigned int time, QsimCircuit* circuit, std::vector* metadata) { unsigned int q0; - bool valid = absl::SimpleAtoi(op.qubits(0).id(), &q0); - if (!valid) { - return Status(absl::StatusCode::kInvalidArgument, - "Could not parse id of qubit 0"); - } - + (void)absl::SimpleAtoi(op.qubits(0).id(), &q0); auto gate = create_f(time, num_qubits - q0 - 1); Status s = OptionalInsertControls(op, num_qubits, &gate); if (!s.ok()) { @@ -187,17 +182,8 @@ inline Status TwoConstantGate( const unsigned int num_qubits, const unsigned int time, QsimCircuit* circuit, std::vector* metadata) { unsigned int q0, q1; - bool valid = absl::SimpleAtoi(op.qubits(0).id(), &q0); - if (!valid) { - return Status(absl::StatusCode::kInvalidArgument, - "Could not parse id of qubit 0"); - } - valid = absl::SimpleAtoi(op.qubits(1).id(), &q1); - if (!valid) { - return Status(absl::StatusCode::kInvalidArgument, - "Could not parse id of qubit 1"); - } - + bool unused = absl::SimpleAtoi(op.qubits(0).id(), &q0); + unused = absl::SimpleAtoi(op.qubits(1).id(), &q1); auto gate = create_f(time, num_qubits - q0 - 1, num_qubits - q1 - 1); Status s = OptionalInsertControls(op, num_qubits, &gate); if (!s.ok()) { @@ -222,13 +208,10 @@ inline Status SingleEigenGate( const unsigned int num_qubits, const unsigned int time, QsimCircuit* circuit, std::vector* metadata) { unsigned int q0; + bool unused; float exp, exp_s, gs; Status u; - bool valid = absl::SimpleAtoi(op.qubits(0).id(), &q0); - if (!valid) { - return Status(absl::StatusCode::kInvalidArgument, - "Could not parse id of qubit 0"); - } + unused = absl::SimpleAtoi(op.qubits(0).id(), &q0); absl::optional exponent_symbol; u = ParseProtoArg(op, "exponent", param_map, &exp, &exponent_symbol); @@ -275,17 +258,10 @@ inline Status TwoEigenGate( QsimCircuit* circuit, std::vector* metadata) { unsigned int q0, q1; float exp, exp_s, gs; + bool unused; Status u; - bool valid = absl::SimpleAtoi(op.qubits(0).id(), &q0); - if (!valid) { - return Status(absl::StatusCode::kInvalidArgument, - "Could not parse id of qubit 0"); - } - valid = absl::SimpleAtoi(op.qubits(1).id(), &q1); - if (!valid) { - return Status(absl::StatusCode::kInvalidArgument, - "Could not parse id of qubit 1"); - } + unused = absl::SimpleAtoi(op.qubits(0).id(), &q0); + unused = absl::SimpleAtoi(op.qubits(1).id(), &q1); absl::optional exponent_symbol; u = ParseProtoArg(op, "exponent", param_map, &exp, &exponent_symbol); @@ -421,13 +397,10 @@ inline Status PhasedXGate(const Operation& op, const SymbolMap& param_map, const unsigned int time, QsimCircuit* circuit, std::vector* metadata) { int q0; + bool unused; float pexp, pexp_s, exp, exp_s, gs; Status u; - bool valid = absl::SimpleAtoi(op.qubits(0).id(), &q0); - if (!valid) { - return Status(absl::StatusCode::kInvalidArgument, - "Could not parse id of qubit 0"); - } + unused = absl::SimpleAtoi(op.qubits(0).id(), &q0); absl::optional exponent_symbol; u = ParseProtoArg(op, "exponent", param_map, &exp, &exponent_symbol); @@ -484,18 +457,11 @@ inline Status FsimGate(const Operation& op, const SymbolMap& param_map, QsimCircuit* circuit, std::vector* metadata) { int q0, q1; + bool unused; float theta, theta_s, phi, phi_s; Status u; - bool valid = absl::SimpleAtoi(op.qubits(0).id(), &q0); - if (!valid) { - return Status(absl::StatusCode::kInvalidArgument, - "Could not parse id of qubit 0"); - } - valid = absl::SimpleAtoi(op.qubits(1).id(), &q1); - if (!valid) { - return Status(absl::StatusCode::kInvalidArgument, - "Could not parse id of qubit 1"); - } + unused = absl::SimpleAtoi(op.qubits(0).id(), &q0); + unused = absl::SimpleAtoi(op.qubits(1).id(), &q1); absl::optional theta_symbol; u = ParseProtoArg(op, "theta", param_map, &theta, &theta_symbol); @@ -548,18 +514,11 @@ inline Status PhasedISwapGate(const Operation& op, const SymbolMap& param_map, const unsigned int time, QsimCircuit* circuit, std::vector* metadata) { int q0, q1; + bool unused; float pexp, pexp_s, exp, exp_s; Status u; - bool valid = absl::SimpleAtoi(op.qubits(0).id(), &q0); - if (!valid) { - return Status(absl::StatusCode::kInvalidArgument, - "Could not parse id of qubit 0"); - } - valid = absl::SimpleAtoi(op.qubits(1).id(), &q1); - if (!valid) { - return Status(absl::StatusCode::kInvalidArgument, - "Could not parse id of qubit 1"); - } + unused = absl::SimpleAtoi(op.qubits(0).id(), &q0); + unused = absl::SimpleAtoi(op.qubits(1).id(), &q1); absl::optional exponent_symbol; u = ParseProtoArg(op, "exponent", param_map, &exp, &exponent_symbol); @@ -646,13 +605,10 @@ inline Status AsymmetricDepolarizingChannel(const Operation& op, const unsigned int time, NoisyQsimCircuit* ncircuit) { int q; + bool unused; float p_x, p_y, p_z; Status u; - bool valid = absl::SimpleAtoi(op.qubits(0).id(), &q); - if (!valid) { - return Status(absl::StatusCode::kInvalidArgument, - "Could not parse id of qubit 0"); - } + unused = absl::SimpleAtoi(op.qubits(0).id(), &q); u = ParseProtoArg(op, "p_x", {}, &p_x); u = ParseProtoArg(op, "p_y", {}, &p_y); @@ -671,13 +627,10 @@ inline Status DepolarizingChannel(const Operation& op, const unsigned int time, NoisyQsimCircuit* ncircuit) { int q; + bool unused; float p; Status u; - bool valid = absl::SimpleAtoi(op.qubits(0).id(), &q); - if (!valid) { - return Status(absl::StatusCode::kInvalidArgument, - "Could not parse id of qubit 0"); - } + unused = absl::SimpleAtoi(op.qubits(0).id(), &q); u = ParseProtoArg(op, "p", {}, &p); if (!u.ok()) { @@ -692,12 +645,10 @@ inline Status DepolarizingChannel(const Operation& op, inline Status GADChannel(const Operation& op, const unsigned int num_qubits, const unsigned int time, NoisyQsimCircuit* ncircuit) { int q; + bool unused; float p, gamma; Status u; - if (!absl::SimpleAtoi(op.qubits(0).id(), &q)) { - return Status(absl::StatusCode::kInvalidArgument, - "Could not parse id of qubit 0"); - } + unused = absl::SimpleAtoi(op.qubits(0).id(), &q); u = ParseProtoArg(op, "p", {}, &p); if (!u.ok()) { @@ -718,10 +669,8 @@ inline Status ResetChannel(const Operation& op, const unsigned int num_qubits, const unsigned int time, NoisyQsimCircuit* ncircuit) { int q; - if (!absl::SimpleAtoi(op.qubits(0).id(), &q)) { - return Status(absl::StatusCode::kInvalidArgument, - "Could not parse id of qubit 0"); - } + bool unused; + unused = absl::SimpleAtoi(op.qubits(0).id(), &q); auto chan = qsim::Cirq::ResetChannel::Create(time, num_qubits - q - 1); ncircuit->channels.push_back(chan); @@ -733,13 +682,10 @@ inline Status AmplitudeDampingChannel(const Operation& op, const unsigned int time, NoisyQsimCircuit* ncircuit) { int q; + bool unused; float gamma; Status u; - bool valid = absl::SimpleAtoi(op.qubits(0).id(), &q); - if (!valid) { - return Status(absl::StatusCode::kInvalidArgument, - "Could not parse id of qubit 0"); - } + unused = absl::SimpleAtoi(op.qubits(0).id(), &q); u = ParseProtoArg(op, "gamma", {}, &gamma); if (!u.ok()) { @@ -756,12 +702,10 @@ inline Status PhaseDampingChannel(const Operation& op, const unsigned int time, NoisyQsimCircuit* ncircuit) { int q; + bool unused; float gamma; Status u; - if (!absl::SimpleAtoi(op.qubits(0).id(), &q)) { - return Status(absl::StatusCode::kInvalidArgument, - "Could not parse id of qubit 0"); - } + unused = absl::SimpleAtoi(op.qubits(0).id(), &q); u = ParseProtoArg(op, "gamma", {}, &gamma); if (!u.ok()) { @@ -779,13 +723,10 @@ inline Status PhaseFlipChannel(const Operation& op, const unsigned int time, NoisyQsimCircuit* ncircuit) { int q; + bool unused; float p; Status u; - bool valid = absl::SimpleAtoi(op.qubits(0).id(), &q); - if (!valid) { - return Status(absl::StatusCode::kInvalidArgument, - "Could not parse id of qubit 0"); - } + unused = absl::SimpleAtoi(op.qubits(0).id(), &q); u = ParseProtoArg(op, "p", {}, &p); if (!u.ok()) { @@ -802,13 +743,10 @@ inline Status BitFlipChannel(const Operation& op, const unsigned int num_qubits, const unsigned int time, NoisyQsimCircuit* ncircuit) { int q; + bool unused; float p; Status u; - bool valid = absl::SimpleAtoi(op.qubits(0).id(), &q); - if (!valid) { - return Status(absl::StatusCode::kInvalidArgument, - "Could not parse id of qubit 0"); - } + unused = absl::SimpleAtoi(op.qubits(0).id(), &q); u = ParseProtoArg(op, "p", {}, &p); if (!u.ok()) { From 9a33e254f893e1e7e070e751fefbf215dde1f3de Mon Sep 17 00:00:00 2001 From: mhucka Date: Fri, 10 Jan 2025 19:26:52 +0000 Subject: [PATCH 5/7] Revert "Add test cases for changes to circuit_parser_qsim.cc" This reverts commit 4853328f7114a3c9173e1e004e9b4d850b518a93. --- .../core/src/circuit_parser_qsim_test.cc | 66 ------------------- 1 file changed, 66 deletions(-) diff --git a/tensorflow_quantum/core/src/circuit_parser_qsim_test.cc b/tensorflow_quantum/core/src/circuit_parser_qsim_test.cc index 7de46aa0a..a1742a205 100644 --- a/tensorflow_quantum/core/src/circuit_parser_qsim_test.cc +++ b/tensorflow_quantum/core/src/circuit_parser_qsim_test.cc @@ -480,38 +480,6 @@ TEST(QsimCircuitParserTest, SingleConstantGate) { } } -TEST(QsimCircuitParserTest, SingleConstantGateBadQubitId) { - Program program_proto; - Circuit* circuit_proto = program_proto.mutable_circuit(); - circuit_proto->set_scheduling_strategy(circuit_proto->MOMENT_BY_MOMENT); - Moment* moments_proto = circuit_proto->add_moments(); - - // Add gate. - Operation* operations_proto = moments_proto->add_operations(); - Gate* gate_proto = operations_proto->mutable_gate(); - gate_proto->set_id("I"); - - // Set the control args to empty. - google::protobuf::Map* args_proto = - operations_proto->mutable_args(); - (*args_proto)["control_qubits"] = MakeControlArg(""); - (*args_proto)["control_values"] = MakeControlArg(""); - - // Set the qubits. - Qubit* qubits_proto = operations_proto->add_qubits(); - qubits_proto->set_id("zero"); - - QsimCircuit test_circuit; - std::vector> fused_circuit; - SymbolMap empty_map; - std::vector metadata; - - ASSERT_EQ(QsimCircuitFromProgram(program_proto, empty_map, 1, &test_circuit, - &fused_circuit, &metadata), - tensorflow::Status(absl::StatusCode::kInvalidArgument, - "Could not parse id of qubit 0")); -} - TEST(QsimCircuitParserTest, SingleConstantGateControlled) { absl::flat_hash_map reference = { {"I", qsim::Cirq::I1::Create(0, 0).ControlledBy({1, 2}, {0, 0})}}; @@ -594,40 +562,6 @@ TEST(QsimCircuitParserTest, TwoConstantGate) { } } -TEST(QsimCircuitParserTest, TwoConstantGateBadQubitId) { - Program program_proto; - Circuit* circuit_proto = program_proto.mutable_circuit(); - circuit_proto->set_scheduling_strategy(circuit_proto->MOMENT_BY_MOMENT); - Moment* moments_proto = circuit_proto->add_moments(); - - // Add gate. - Operation* operations_proto = moments_proto->add_operations(); - Gate* gate_proto = operations_proto->mutable_gate(); - gate_proto->set_id("I2"); - - // Set the control args to empty. - google::protobuf::Map* args_proto = - operations_proto->mutable_args(); - (*args_proto)["control_qubits"] = MakeControlArg(""); - (*args_proto)["control_values"] = MakeControlArg(""); - - QsimCircuit test_circuit; - std::vector> fused_circuit; - SymbolMap empty_map; - std::vector metadata; - - // Set the qubit id's, but use a bad value for the 2nd one. - Qubit* qubits_proto = operations_proto->add_qubits(); - qubits_proto->set_id("0"); - qubits_proto = operations_proto->add_qubits(); - qubits_proto->set_id("one"); - - ASSERT_EQ(QsimCircuitFromProgram(program_proto, empty_map, 2, &test_circuit, - &fused_circuit, &metadata), - tensorflow::Status(absl::StatusCode::kInvalidArgument, - "Could not parse id of qubit 1")); -} - TEST(QsimCircuitParserTest, TwoConstantGateControlled) { absl::flat_hash_map reference = { {"I2", From 67b28fd12f592c8125aa4ac4efec7690a802fe93 Mon Sep 17 00:00:00 2001 From: mhucka Date: Fri, 10 Jan 2025 19:29:21 +0000 Subject: [PATCH 6/7] Replace variable "unused" with explicit void casts Per discussion with Michael B. on an earlier version of this PR, I replaced the assignments with simple (void) casts. --- .../core/src/circuit_parser_qsim.cc | 49 +++++++------------ 1 file changed, 18 insertions(+), 31 deletions(-) diff --git a/tensorflow_quantum/core/src/circuit_parser_qsim.cc b/tensorflow_quantum/core/src/circuit_parser_qsim.cc index 2b3e81d19..e63235dd2 100644 --- a/tensorflow_quantum/core/src/circuit_parser_qsim.cc +++ b/tensorflow_quantum/core/src/circuit_parser_qsim.cc @@ -182,8 +182,8 @@ inline Status TwoConstantGate( const unsigned int num_qubits, const unsigned int time, QsimCircuit* circuit, std::vector* metadata) { unsigned int q0, q1; - bool unused = absl::SimpleAtoi(op.qubits(0).id(), &q0); - unused = absl::SimpleAtoi(op.qubits(1).id(), &q1); + (void) absl::SimpleAtoi(op.qubits(0).id(), &q0); + (void) absl::SimpleAtoi(op.qubits(1).id(), &q1); auto gate = create_f(time, num_qubits - q0 - 1, num_qubits - q1 - 1); Status s = OptionalInsertControls(op, num_qubits, &gate); if (!s.ok()) { @@ -208,10 +208,9 @@ inline Status SingleEigenGate( const unsigned int num_qubits, const unsigned int time, QsimCircuit* circuit, std::vector* metadata) { unsigned int q0; - bool unused; float exp, exp_s, gs; Status u; - unused = absl::SimpleAtoi(op.qubits(0).id(), &q0); + (void) absl::SimpleAtoi(op.qubits(0).id(), &q0); absl::optional exponent_symbol; u = ParseProtoArg(op, "exponent", param_map, &exp, &exponent_symbol); @@ -258,10 +257,9 @@ inline Status TwoEigenGate( QsimCircuit* circuit, std::vector* metadata) { unsigned int q0, q1; float exp, exp_s, gs; - bool unused; Status u; - unused = absl::SimpleAtoi(op.qubits(0).id(), &q0); - unused = absl::SimpleAtoi(op.qubits(1).id(), &q1); + (void) absl::SimpleAtoi(op.qubits(0).id(), &q0); + (void) absl::SimpleAtoi(op.qubits(1).id(), &q1); absl::optional exponent_symbol; u = ParseProtoArg(op, "exponent", param_map, &exp, &exponent_symbol); @@ -397,10 +395,9 @@ inline Status PhasedXGate(const Operation& op, const SymbolMap& param_map, const unsigned int time, QsimCircuit* circuit, std::vector* metadata) { int q0; - bool unused; float pexp, pexp_s, exp, exp_s, gs; Status u; - unused = absl::SimpleAtoi(op.qubits(0).id(), &q0); + (void) absl::SimpleAtoi(op.qubits(0).id(), &q0); absl::optional exponent_symbol; u = ParseProtoArg(op, "exponent", param_map, &exp, &exponent_symbol); @@ -457,11 +454,10 @@ inline Status FsimGate(const Operation& op, const SymbolMap& param_map, QsimCircuit* circuit, std::vector* metadata) { int q0, q1; - bool unused; float theta, theta_s, phi, phi_s; Status u; - unused = absl::SimpleAtoi(op.qubits(0).id(), &q0); - unused = absl::SimpleAtoi(op.qubits(1).id(), &q1); + (void) absl::SimpleAtoi(op.qubits(0).id(), &q0); + (void) absl::SimpleAtoi(op.qubits(1).id(), &q1); absl::optional theta_symbol; u = ParseProtoArg(op, "theta", param_map, &theta, &theta_symbol); @@ -514,11 +510,10 @@ inline Status PhasedISwapGate(const Operation& op, const SymbolMap& param_map, const unsigned int time, QsimCircuit* circuit, std::vector* metadata) { int q0, q1; - bool unused; float pexp, pexp_s, exp, exp_s; Status u; - unused = absl::SimpleAtoi(op.qubits(0).id(), &q0); - unused = absl::SimpleAtoi(op.qubits(1).id(), &q1); + (void) absl::SimpleAtoi(op.qubits(0).id(), &q0); + (void) absl::SimpleAtoi(op.qubits(1).id(), &q1); absl::optional exponent_symbol; u = ParseProtoArg(op, "exponent", param_map, &exp, &exponent_symbol); @@ -605,10 +600,9 @@ inline Status AsymmetricDepolarizingChannel(const Operation& op, const unsigned int time, NoisyQsimCircuit* ncircuit) { int q; - bool unused; float p_x, p_y, p_z; Status u; - unused = absl::SimpleAtoi(op.qubits(0).id(), &q); + (void) absl::SimpleAtoi(op.qubits(0).id(), &q); u = ParseProtoArg(op, "p_x", {}, &p_x); u = ParseProtoArg(op, "p_y", {}, &p_y); @@ -627,10 +621,9 @@ inline Status DepolarizingChannel(const Operation& op, const unsigned int time, NoisyQsimCircuit* ncircuit) { int q; - bool unused; float p; Status u; - unused = absl::SimpleAtoi(op.qubits(0).id(), &q); + (void) absl::SimpleAtoi(op.qubits(0).id(), &q); u = ParseProtoArg(op, "p", {}, &p); if (!u.ok()) { @@ -645,10 +638,9 @@ inline Status DepolarizingChannel(const Operation& op, inline Status GADChannel(const Operation& op, const unsigned int num_qubits, const unsigned int time, NoisyQsimCircuit* ncircuit) { int q; - bool unused; float p, gamma; Status u; - unused = absl::SimpleAtoi(op.qubits(0).id(), &q); + (void) absl::SimpleAtoi(op.qubits(0).id(), &q); u = ParseProtoArg(op, "p", {}, &p); if (!u.ok()) { @@ -669,8 +661,7 @@ inline Status ResetChannel(const Operation& op, const unsigned int num_qubits, const unsigned int time, NoisyQsimCircuit* ncircuit) { int q; - bool unused; - unused = absl::SimpleAtoi(op.qubits(0).id(), &q); + (void) absl::SimpleAtoi(op.qubits(0).id(), &q); auto chan = qsim::Cirq::ResetChannel::Create(time, num_qubits - q - 1); ncircuit->channels.push_back(chan); @@ -682,10 +673,9 @@ inline Status AmplitudeDampingChannel(const Operation& op, const unsigned int time, NoisyQsimCircuit* ncircuit) { int q; - bool unused; float gamma; Status u; - unused = absl::SimpleAtoi(op.qubits(0).id(), &q); + (void) absl::SimpleAtoi(op.qubits(0).id(), &q); u = ParseProtoArg(op, "gamma", {}, &gamma); if (!u.ok()) { @@ -702,10 +692,9 @@ inline Status PhaseDampingChannel(const Operation& op, const unsigned int time, NoisyQsimCircuit* ncircuit) { int q; - bool unused; float gamma; Status u; - unused = absl::SimpleAtoi(op.qubits(0).id(), &q); + (void) absl::SimpleAtoi(op.qubits(0).id(), &q); u = ParseProtoArg(op, "gamma", {}, &gamma); if (!u.ok()) { @@ -723,10 +712,9 @@ inline Status PhaseFlipChannel(const Operation& op, const unsigned int time, NoisyQsimCircuit* ncircuit) { int q; - bool unused; float p; Status u; - unused = absl::SimpleAtoi(op.qubits(0).id(), &q); + (void) absl::SimpleAtoi(op.qubits(0).id(), &q); u = ParseProtoArg(op, "p", {}, &p); if (!u.ok()) { @@ -743,10 +731,9 @@ inline Status BitFlipChannel(const Operation& op, const unsigned int num_qubits, const unsigned int time, NoisyQsimCircuit* ncircuit) { int q; - bool unused; float p; Status u; - unused = absl::SimpleAtoi(op.qubits(0).id(), &q); + (void) absl::SimpleAtoi(op.qubits(0).id(), &q); u = ParseProtoArg(op, "p", {}, &p); if (!u.ok()) { From 0b216e4496adbfe317bc486a2924c5f6b7d41e97 Mon Sep 17 00:00:00 2001 From: mhucka Date: Fri, 10 Jan 2025 19:47:42 +0000 Subject: [PATCH 7/7] Address code format issues I guess the style is "(void)foo" and not "(void) foo". --- .../core/src/circuit_parser_qsim.cc | 36 +++++++++---------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/tensorflow_quantum/core/src/circuit_parser_qsim.cc b/tensorflow_quantum/core/src/circuit_parser_qsim.cc index e63235dd2..3f180b426 100644 --- a/tensorflow_quantum/core/src/circuit_parser_qsim.cc +++ b/tensorflow_quantum/core/src/circuit_parser_qsim.cc @@ -182,8 +182,8 @@ inline Status TwoConstantGate( const unsigned int num_qubits, const unsigned int time, QsimCircuit* circuit, std::vector* metadata) { unsigned int q0, q1; - (void) absl::SimpleAtoi(op.qubits(0).id(), &q0); - (void) absl::SimpleAtoi(op.qubits(1).id(), &q1); + (void)absl::SimpleAtoi(op.qubits(0).id(), &q0); + (void)absl::SimpleAtoi(op.qubits(1).id(), &q1); auto gate = create_f(time, num_qubits - q0 - 1, num_qubits - q1 - 1); Status s = OptionalInsertControls(op, num_qubits, &gate); if (!s.ok()) { @@ -210,7 +210,7 @@ inline Status SingleEigenGate( unsigned int q0; float exp, exp_s, gs; Status u; - (void) absl::SimpleAtoi(op.qubits(0).id(), &q0); + (void)absl::SimpleAtoi(op.qubits(0).id(), &q0); absl::optional exponent_symbol; u = ParseProtoArg(op, "exponent", param_map, &exp, &exponent_symbol); @@ -258,8 +258,8 @@ inline Status TwoEigenGate( unsigned int q0, q1; float exp, exp_s, gs; Status u; - (void) absl::SimpleAtoi(op.qubits(0).id(), &q0); - (void) absl::SimpleAtoi(op.qubits(1).id(), &q1); + (void)absl::SimpleAtoi(op.qubits(0).id(), &q0); + (void)absl::SimpleAtoi(op.qubits(1).id(), &q1); absl::optional exponent_symbol; u = ParseProtoArg(op, "exponent", param_map, &exp, &exponent_symbol); @@ -397,7 +397,7 @@ inline Status PhasedXGate(const Operation& op, const SymbolMap& param_map, int q0; float pexp, pexp_s, exp, exp_s, gs; Status u; - (void) absl::SimpleAtoi(op.qubits(0).id(), &q0); + (void)absl::SimpleAtoi(op.qubits(0).id(), &q0); absl::optional exponent_symbol; u = ParseProtoArg(op, "exponent", param_map, &exp, &exponent_symbol); @@ -456,8 +456,8 @@ inline Status FsimGate(const Operation& op, const SymbolMap& param_map, int q0, q1; float theta, theta_s, phi, phi_s; Status u; - (void) absl::SimpleAtoi(op.qubits(0).id(), &q0); - (void) absl::SimpleAtoi(op.qubits(1).id(), &q1); + (void)absl::SimpleAtoi(op.qubits(0).id(), &q0); + (void)absl::SimpleAtoi(op.qubits(1).id(), &q1); absl::optional theta_symbol; u = ParseProtoArg(op, "theta", param_map, &theta, &theta_symbol); @@ -512,8 +512,8 @@ inline Status PhasedISwapGate(const Operation& op, const SymbolMap& param_map, int q0, q1; float pexp, pexp_s, exp, exp_s; Status u; - (void) absl::SimpleAtoi(op.qubits(0).id(), &q0); - (void) absl::SimpleAtoi(op.qubits(1).id(), &q1); + (void)absl::SimpleAtoi(op.qubits(0).id(), &q0); + (void)absl::SimpleAtoi(op.qubits(1).id(), &q1); absl::optional exponent_symbol; u = ParseProtoArg(op, "exponent", param_map, &exp, &exponent_symbol); @@ -602,7 +602,7 @@ inline Status AsymmetricDepolarizingChannel(const Operation& op, int q; float p_x, p_y, p_z; Status u; - (void) absl::SimpleAtoi(op.qubits(0).id(), &q); + (void)absl::SimpleAtoi(op.qubits(0).id(), &q); u = ParseProtoArg(op, "p_x", {}, &p_x); u = ParseProtoArg(op, "p_y", {}, &p_y); @@ -623,7 +623,7 @@ inline Status DepolarizingChannel(const Operation& op, int q; float p; Status u; - (void) absl::SimpleAtoi(op.qubits(0).id(), &q); + (void)absl::SimpleAtoi(op.qubits(0).id(), &q); u = ParseProtoArg(op, "p", {}, &p); if (!u.ok()) { @@ -640,7 +640,7 @@ inline Status GADChannel(const Operation& op, const unsigned int num_qubits, int q; float p, gamma; Status u; - (void) absl::SimpleAtoi(op.qubits(0).id(), &q); + (void)absl::SimpleAtoi(op.qubits(0).id(), &q); u = ParseProtoArg(op, "p", {}, &p); if (!u.ok()) { @@ -661,7 +661,7 @@ inline Status ResetChannel(const Operation& op, const unsigned int num_qubits, const unsigned int time, NoisyQsimCircuit* ncircuit) { int q; - (void) absl::SimpleAtoi(op.qubits(0).id(), &q); + (void)absl::SimpleAtoi(op.qubits(0).id(), &q); auto chan = qsim::Cirq::ResetChannel::Create(time, num_qubits - q - 1); ncircuit->channels.push_back(chan); @@ -675,7 +675,7 @@ inline Status AmplitudeDampingChannel(const Operation& op, int q; float gamma; Status u; - (void) absl::SimpleAtoi(op.qubits(0).id(), &q); + (void)absl::SimpleAtoi(op.qubits(0).id(), &q); u = ParseProtoArg(op, "gamma", {}, &gamma); if (!u.ok()) { @@ -694,7 +694,7 @@ inline Status PhaseDampingChannel(const Operation& op, int q; float gamma; Status u; - (void) absl::SimpleAtoi(op.qubits(0).id(), &q); + (void)absl::SimpleAtoi(op.qubits(0).id(), &q); u = ParseProtoArg(op, "gamma", {}, &gamma); if (!u.ok()) { @@ -714,7 +714,7 @@ inline Status PhaseFlipChannel(const Operation& op, int q; float p; Status u; - (void) absl::SimpleAtoi(op.qubits(0).id(), &q); + (void)absl::SimpleAtoi(op.qubits(0).id(), &q); u = ParseProtoArg(op, "p", {}, &p); if (!u.ok()) { @@ -733,7 +733,7 @@ inline Status BitFlipChannel(const Operation& op, const unsigned int num_qubits, int q; float p; Status u; - (void) absl::SimpleAtoi(op.qubits(0).id(), &q); + (void)absl::SimpleAtoi(op.qubits(0).id(), &q); u = ParseProtoArg(op, "p", {}, &p); if (!u.ok()) {