ngnrsaa / qflex

Flexible Quantum Circuit Simulator (qFlex) implements an efficient tensor network, CPU-based simulator of large quantum circuits.
Apache License 2.0
97 stars 24 forks source link

Try-catch for evaluate_circuit.cpp and contraction_utils.cpp #231

Closed alankao64 closed 4 years ago

alankao64 commented 4 years ago

Added try-catch for calls to methods in evaluate_circuit.cpp and contration_utils.cpp that can throw exceptions.

alankao64 commented 4 years ago

Still in draft, don't review just yet.

alankao64 commented 4 years ago

changed const std::string to std::string moved variable declarations outside try-catch blocks

s-mandra commented 4 years ago

For some reasons, evaluate_circuit_test.cpp doesn't like this ordering anymore:

cut () 1 3
expand a 1
expand a 0
expand a 2
cut () 5
expand b 5
expand b 3
merge a b

any thoughts of the reason why?

s-mandra commented 4 years ago

For some reasons, evaluate_circuit_test.cpp doesn't like this ordering anymore:

cut () 1 3
expand a 1
expand a 0
expand a 2
cut () 5
expand b 5
expand b 3
merge a b

any thoughts of the reason why?

ERROR (contraction_utils.cpp: ordering_data_to_contraction_ordering:471) --> 
Index (0,1),(1,1) is cut multiple times.
Tensor at (0,1) is added to non-empty patch a after a cut.
Tensor at (0,1) is added to non-empty patch a after a cut.
Tensor (0,1) is contracted multiple times.
Tensor at (0,0) is added to non-empty patch a after a cut.
Tensor at (0,0) is added to non-empty patch a after a cut.
Tensor (0,0) is contracted multiple times.
Tensor at (1,0) is added to non-empty patch a after a cut.
Tensor at (1,0) is added to non-empty patch a after a cut.
Tensor (1,0) is contracted multiple times.
Index (2,1),(o) is cut multiple times.
Tensor at (2,1) is added to non-empty patch b after a cut.
Tensor (2,1) is contracted multiple times.
Tensor at (1,1) is added to non-empty patch b after a cut.
Tensor (1,1) is contracted multiple times.
Patch a is merged multiple times.
Patch a is merged into non-empty patch b after a cut.
95-martin-orion commented 4 years ago

For some reasons, evaluate_circuit_test.cpp doesn't like this ordering anymore: {...ordering...} any thoughts of the reason why?

Pretty sure this is caused by the duplicate ordering_data_to_contraction_ordering call in evaluate_circuit.cpp:135-143.

s-mandra commented 4 years ago

For some reasons, evaluate_circuit_test.cpp doesn't like this ordering anymore: {...ordering...} any thoughts of the reason why?

Pretty sure this is caused by the duplicate ordering_data_to_contraction_ordering call in evaluate_circuit.cpp:135-143.

Thanks! I was wondering if I made a mistake once I merged master to try-catch (other examples were indeed broken!)

s-mandra commented 4 years ago

For some reasons, evaluate_circuit_test.cpp doesn't like this ordering anymore: {...ordering...} any thoughts of the reason why?

Pretty sure this is caused by the duplicate ordering_data_to_contraction_ordering call in evaluate_circuit.cpp:135-143.

Everything should work now!

s-mandra commented 4 years ago

See comments - based on the scope of index_name return values, we probably need to treat it as a "fundamental" function. @s-mandra, does this seem reasonable?

Just had a discussion with @alankao64. Given that CUT is the only function that calls index_name, perhaps it makes sense to have index_name to just return an empty string in case of error and let CUT handle the error. @95-martin-orion, what is your opinion? Thanks!

95-martin-orion commented 4 years ago

{...} Given that CUT is the only function that calls index_name, perhaps it makes sense to have index_name to just return an empty string in case of error and let CUT handle the error.

read_circuit.cpp also makes calls to index_name. Since we probably want a similar error message at both call sites, I'd prefer throwing the error in index_name and leaving it uncaught in ContractionData::ContractGrid. The error will percolate up and be caught in the public ContractGrid method (line ~600), which should provide enough detail for users.

s-mandra commented 4 years ago

{...} Given that CUT is the only function that calls index_name, perhaps it makes sense to have index_name to just return an empty string in case of error and let CUT handle the error.

read_circuit.cpp also makes calls to index_name. Since we probably want a similar error message at both call sites, I'd prefer throwing the error in index_name and leaving it uncaught in ContractionData::ContractGrid. The error will percolate up and be caught in the public ContractGrid method (line ~600), which should provide enough detail for users.

Sounds good to me!

s-mandra commented 4 years ago

@alankao64, remember to catch reference to constant strings as const std::string& msg, not strings.

alankao64 commented 4 years ago

@alankao64, remember to catch reference to constant strings as const std::string& msg, not strings.

Got it, sorry about that!

s-mandra commented 4 years ago

Looks good to me! In a follow-up PR, let's update contraction_utils_test to use more specific exception testing in place of EXPECT_ANY_THROW.

Fixed.

s-mandra commented 4 years ago

At the moment, ordering_data_to_contraction_ordering doesn't properly rethrow errors and some useful info are just printed to std::cerr. This requires some extra changes in evaluate_circuit.cpp, so I'll open a new PR for this.

s-mandra commented 4 years ago

At the moment, ordering_data_to_contraction_ordering doesn't properly rethrow errors and some useful info are just printed to std::cerr. This requires some extra changes in evaluate_circuit.cpp, so I'll open a new PR for this.

Fixed in #248.