Refactor Shor's algorithm

[MetcalfeTom]

Fixes #933

Summary

• phi_add() was casted to a Gate instance, allowing .inverse() and .control() methods to be used during the construction of the circuit in Shor's algorithm
• A ParameterVector was used during circuit construction to store the phi angles
• Subsequently redundant control and inverse methods were removed from Shor
• Generic rotational gates were replaced with more familiar notation (H, X) where possible
• Type hints were added to the internal methods of Shor
• The run method now returns a AlgorithmResult instance instead of a dictionary
• Failed results are no longer gathered and returned from run, rather just a count of the total/successful factorizations after executing the quantum circuit

Details and comments

I think it might also be possible to cast the two remaining controlled methods to Gate instances as well, then explicitly call .control(). However since they are parametrised by a (and the ParameterVector does not support enough complex computations to be used) the gates would have to be recreated multiple times when constructing the circuit, so they have been kept as circuit-level functions.

[CLAassistant]

All committers have signed the CLA.

[Cryoris]

Hi @MetcalfeTom,

thanks for the work! I've left a few style comments and I've got two more points I think we should address:

1. You're giving names to the operations but they are lost as we use compose, since this method adds the contents of the appended circuit directly. By using append(circuit.to_instruction) we can keep the label. I think we should do this, since then printing the circuit is much more organized and people can actually see a circuit that resembles the paper instead of just seeing standard gate operations.

           ┌───┐┌──────────────────────┐      ┌────────────────────────┐  
     up_0: ┤ H ├┤0                     ├─■──■─┤0            
           ├───┤│                      │ │  │ │                
     up_1: ┤ H ├┤1                     ├─┼──┼─┤1               
           ├───┤│                      │ │  │ │                 
     up_2: ┤ H ├┤2                     ├─┼──┼─┤2               
           ├───┤│                      │ │  │ │          
     up_3: ┤ H ├┤3                     ├─┼──┼─┤3                    
           ├───┤│                      │ │  │ │               
   down_0: ┤ X ├┤4                     ├─X──┼─┤4           
           └───┘│  multiply_by_2_mod_3 │ │  │ │  multiply_by_2_mod_3_dg
   down_1: ─────┤5                     ├─┼──X─┤5         
                │                      │ │  │ │             
    aux_0: ─────┤6                     ├─X──┼─┤6          
                │                      │    │ │                
    aux_1: ─────┤7                     ├────X─┤7        
                │                      │      │                  
    aux_2: ─────┤8                     ├──────┤8          
                │                      │      │               
    aux_3: ─────┤9                     ├──────┤9                    
                └──────────────────────┘      └────────────────────────┘
   

   Also it seems that compose is slower than append after doing some timings, sorry for proposing it in the first place! Thoughts?

2. I've looked at rebuilding the circuits again and I think we can do this better after all by using parameter binds. Conceptually this is what I'm thinking

   angle_params = ParameterVector('angles', len=self._n + 1)
   controlled_controlled_phi_add_N = .. # build once using the parameters angle_params
   for i, ctl_up in ... :
      angles = self._get_angles(a)
      bound = controlled_controlled_phi_add_N.assign_parameters({angle_params: angles})
      # append `bound` to the circuit

   So don't compute the angles for the value a, but parameterize the circuit on these angles. Then we can bind the correct angles when we add the circuit. Does that make sense?

[MetcalfeTom]

@Cryoris Thanks for reviewing my code

I will take on the suggestion to cast to instruction and append in order to keep the circuit drawings more perspicuous. Though I think that it's a little misleading that these instructions lie across all qubits in the up register in the drawing when only one is used. It is still more readable in any case.

Thanks for the tip about the ParameterVector! I will give it a go and let you know if I have any difficulties with it

[MetcalfeTom]

Just so I can follow, you'd want the results to now look like:

{
  "factors": [
    [3, 5]
  ],
  "results": {
    "11011010": (5, 3),
    "11101011": (5, 3),
   ...
  }
}

i.e. do not append the error messages to the AlgorithmResult?

In that case I would propose altering the output of _get_factors() to Union[Tuple[int, int], str] (i.e. the factors or error message). That way during _run() we can append the factors to the result and log the failures under debug level, counting them for the final x of y counts processed successfully message.

[woodsp-ibm]

As you are refactoring the algorithm I guess the question would be whether all that detail in the "results", whether an error or factors found is meaningful in anyway as a return. Having it all logged under debug, whether an error or success would allow anyone wanting to see more detail to have it without having a large unwieldy result object.
The x of y counts was just to give some overall indication, since that results did exist, but again it may have little end user value.

[Cryoris]

If we put additional information I think some data about the complexity is most interesting. Giving the counts and successful counts already somewhat does that, so we'd have something like:

{
  "factors": [
    [3, 5]
  ],
  "total_counts": x,
  "successful_counts": x,
}

Is there some other information we could include?

Since we already return a dict/AlgorithmResult I think its fair to add some information. Otherwise, if it just contains the factors, we could also return them (as list). But that would be an inconsistent return type across the other algos.

[woodsp-ibm]

That return format seems quite reasonable. Lets go with that.

When we more broadly update the algorithms with interface types and return result we may want a FactorizerResult with some common methods to access result parts and the above count information may be part of some ShorResult subclass thereof. For now lets keep things more as they are and go with the return format as proposed above.

[MetcalfeTom]

@woodsp-ibm, @Cryoris I addressed the changes.

Since we are no longer gathering the reasons for failure, I tried to clean up the _get_factors() logic a little too.

[Cryoris]

I think this is good to go! Thanks a lot for the refactor, the constructed circuit looks much nicer now and can be decomposed step-by-step and we're up-to-date with current Terra functionality.
Great work 👍

[woodsp-ibm]

@MetcalfeTom Thanks for undertaking this refactoring and completing the work. It's much appreciated and it's great to have the algorithm refreshed and brought more up to date.

[MetcalfeTom]

Thanks for the guidance @Cryoris @woodsp-ibm ! On to the next one 😊