amazon-braket-sdk-python icon indicating copy to clipboard operation
amazon-braket-sdk-python copied to clipboard

Add circuit loader methods: from_ir, from_repr and from_diagram

Open kjacky opened this issue 4 years ago • 3 comments

Issue #, if available:

Description of changes: Add three circuit loader methods to the Circuit class:

  • from_ir(program: Program) -> Circuit - Create Circuit from Program object.
  • from_repr(repr_str: str) -> Circuit - Create Circuit from a string produced by repr(circuit).
  • from_diagram(diagram_str: str) -> Circuit - Create Circuit from a circuit diagram string produced by f'{circuit}'.

These methods are added to the Circuit class as a subroutine using @circuit.subroutine(register=True).

Testing done:

  • ./test/unit_tests/braket/circuits/test_from.py to test 30 test cases of circuit ascii diagrams covering quantum gates, noise operators and result types. (The ascii diagram files are stored under from/input/test_cases.txt.)
  • ./test/unit_tests/braket/circuits/test_from_matrics.py to test the unitary gate, the Kraus noise operator and the Hermitian result type.
  • ./test/unit_tests/braket/circuits/test_from_xfail.py to test error cases.

Merge Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.

General

  • [x] I have read the CONTRIBUTING doc
  • [x] I used the commit message format described in CONTRIBUTING
  • [x] I have updated any necessary documentation, including READMEs and API docs (if appropriate)

Tests

  • [x] I have added tests that prove my fix is effective or that my feature works (if appropriate)
  • [x] I have checked that my tests are not configured for a specific region or account (if appropriate)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

kjacky avatar Jul 02 '21 12:07 kjacky

Thanks for the PR @kjacky! This would be a very useful addition. Following are some initial thoughts:

  • It'd be nice to break out the PR to focus on a single type of translation. This would help make it easier to review the changes. I'd recommend starting with from_ir() for the following reasons:

    • The IR contains the complete representation of the circuit. Other representations would truncate information (such as the exact value for an angle in an angled gate) or mask it (Unitary gate is represented as "Unitary" in an ASCII diagram and doesn't contain the information about the unitary matrix being used). This would cause the from_repr and from_diagram to not work in all cases.
    • The IR can always be retrieved post-execution (from the task execution results). The circuit repr and diagram are not stored by default. Hence, a customer won't be able to use the from_repr and from_diagram unless they explicitly store these, and if they already have a way to reconstruct the circuit from the stored representation (IR) it's questionable what value these methods would add.
    • Methods such as from_diagram can be very brittle since they rely on correctly parsing extraneous formatting of the circuit layout. The circuit formatting can change frequently causing these methods to break.
  • The current from_ir implementation is too complex (which you might have seen the build failure complaining about as well). We should try to think about how to simplify this. Here are a couple of ideas which might be worth considering:

    • The current implementation aggregates all the logic across multiple object types in a single method. This would be hard to maintain and reason about as we add in new object types. One way to solve this would be to delegate the responsibility to the different object types themselves. You might have seen that each class (Circuit, Instruction, Gate, ResultType) have a to_ir() method which contains logic on how that specific object can be converted to the IR representation, and the classes delegate this conversion where possible (Circuit delegates the IR conversion for instructions to the Instruction class, which in turn delegates the conversion for the quantum operator to the specific operator class). Similarly, each class could have a from_ir() method which knows how to construct an instance of the specific class from the IR representation.
    class Circuit:
    
      @classmethod
      def from_ir(cls, program: Program) -> Circuit:
         instructions = [Instruction.from_ir(program.instructions)]
         results = [ResultType.from_ir(program.results)]
         return cls(instructions + results)
    
    • We are creating some duplicated mappings obj_class_defs, gate_defs, result_type_defs, observable_defs, gate_to_observable. Consider how we could reuse the existing mappings to serve this use case. It might help to look at how some of the classes are storing this information using the register methods. (See Gate, Noise, ResultType)

kshitijc avatar Jul 07 '21 20:07 kshitijc

Codecov Report

Merging #256 (f5d810e) into main (9c4f94d) will not change coverage. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##              main      #256    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           41        42     +1     
  Lines         3017      3140   +123     
  Branches       414       424    +10     
==========================================
+ Hits          3017      3140   +123     
Impacted Files Coverage Δ
src/braket/circuits/observables.py 100.00% <ø> (ø)
src/braket/circuits/angled_gate.py 100.00% <100.00%> (ø)
src/braket/circuits/circuit.py 100.00% <100.00%> (ø)
src/braket/circuits/circuit_utils.py 100.00% <100.00%> (ø)
src/braket/circuits/gate.py 100.00% <100.00%> (ø)
src/braket/circuits/gates.py 100.00% <100.00%> (ø)
src/braket/circuits/instruction.py 100.00% <100.00%> (ø)
src/braket/circuits/noise.py 100.00% <100.00%> (ø)
src/braket/circuits/noises.py 100.00% <100.00%> (ø)
src/braket/circuits/result_type.py 100.00% <100.00%> (ø)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 9c4f94d...f5d810e. Read the comment docs.

codecov[bot] avatar Jul 13 '21 06:07 codecov[bot]

Thank you very much @kshitijc for your guidance. I have removed from_diagram and from_repr, then refactored from_ir into individual braket.circuits classes. All *_defs and conversions were removed.

Each of the 16 from_ir() class methods returns an object of the cls type. All are 1-to-3 liners, except for Instruction::from_ir() with 15 non-comment lines implementing the core logic. Tests and coverage were passed 100%.

I've added two utility functions for internal use: complex_matrices() and _attr_dict() (near the bottom of circuits/result_type.py for lack of a better place I found - feel free to move them).

Looking forward to your feedback @kshitijc .

kjacky avatar Jul 13 '21 06:07 kjacky

Closing PR since there has been no activity on this for a while. Please feel free to reopen if you wish to continue working on this. Thanks!

kshitijc avatar May 24 '23 22:05 kshitijc