Skip to content
This repository has been archived by the owner on Dec 7, 2021. It is now read-only.

Operator.compose should allow indices #1144

Merged
merged 126 commits into from
Sep 25, 2020

Conversation

molar-volume
Copy link
Contributor

@molar-volume molar-volume commented Jul 27, 2020

resolves #1088
resolves #1143

This PR is created to verify the solution for expanding operator in #1088 (permute part of issue is not addressed yet)

The method expand for expanding the operator with identity is defined in OperatorBase,
because it enhances compose (defined for OperatorBase),
and uses tensor (defined for OperatorBase), and new method identity (abstract in OperatorBase).

identity is implemented for each subclass of 'OperatorBase',

but I am unsure of implementation for DictStateFn and VectorStateFn (instead of identity, zeros are used).

The new features are demonstrated intest_expand_on_state_fn and test_compose_op_of_different_dim

Summary

Progress for #1088 and solution for #1143

Details and comments

merge qiskit-aqua to my fork
…at it expands shorter operator with identity

2) implemented OperatorBase.expand
3) defined abstract method OperatorBase.identity, and implemented in child classes of OperatorBase (TODO check if implementation makes sense for every class)
4) created tests for composing Operators of different dimensions
…ays default to False), even if tensored instances were is_measurement=True
@molar-volume molar-volume changed the title Issue 1088 Operator.compose should allow indices Jul 27, 2020
@Cryoris
Copy link
Contributor

Cryoris commented Jul 27, 2020

Thanks for the commitment @molar-volume! I think we need to clarify what #1088 should do and how we could do it. The idea was that you could pass concrete indices (not only ops of different sizes) and e.g. do

op = (X ^ 5)
op.compose( Z ^ Y, qubits=[0, 3], inplace=True)
print(op)  # ComposedOp( [X ^ X ^ X ^ X ^ X], [Z ^ I ^ I ^ Y ^ I] )

So it is closer to the same method of the quantum circuit, see QuantumCircuit.compose. (You could also add the front feature if you want! 🙂 ).

The approach in here does only allow for consecutive qubit indices (e.g. [1,2,3]) not for others (such as [0, 3]) if I understand it correctly. I have two concerns about the methods you added:

  • expand already exists in the quantum_info operators but has a different signature. There you can call Operator.expand(other_operator_of_same_dim), I think we should keep the same syntax if possible and therefore not call it expand here. If this turns out to be necessary we could possibly call it expand_to_dim or pad_with_identities or so.
  • Adding an identity method is maybe not the best approach since it is not clear to me what you would expect upon calling CircuitOp.identity? On a circuit this adds an identity gate, here it does something different. You could hide this inside pad_with_identities.

A possible approach would be to add a pad_with_identities (or whatever you want to call it) method looking like

def pad_with_identities(self, new_dim, qargs=None):
    """Extend this operator to ``new_dim`` where this operator acts on ``qargs``.

    Args:
        new_dim: The new dimension, must be larger equal than ``self.num_qubits``.
        qargs: The qubit indices this operator acts on, must be a list of length ``self.num_qubits``.
    """

And then in compose you can use the above to expand other to the correct size and compose it. What do you think?

@molar-volume
Copy link
Contributor Author

molar-volume commented Jul 27, 2020

Hi @Cryoris thank you for the feedback !
Yes, let’s rename expand to expand_to_dim.

Now identity (or we should rather name it identity_operator) is OperatorBase with primitive corresponding to identity matrix, so for CircuitOp, it returns (I ^ num_qubits).to_circuit_op() .

I think it has some advantages, for example you can define expand_to_dim for all subclasses in OperatorBase as

def expand_to_dim(self, num_qubits: int) -> OperatorBase:
	return self.tensor(self.identity_operator(num_qubits))

I understand that we need a more general method with indices, just like you defined

def pad_with_identities(self, new_dim, qargs=None)

For the CircuitOp, that functionality can be already provided using permute (as suggested by @dongreenberg )

circ_op = (X ^ Y).to_circuit_op().permute([0,2]) @ (X ^ Y ^ Z ^ X).to_circuit_op()

print(circ_op.to_circuit())
     ┌───┐     
q_0: ┤ X ├─────
     ├───┤┌───┐
q_1: ┤ Z ├┤ Y ├
     ├───┤└───┘
q_2: ┤ Y ├─────
     ├───┤┌───┐
q_3: ┤ X ├┤ X ├
     └───┘└───┘

I think I know how to implement permute for PauliOp, but do you have any suggestion how to do it for MatrixOp?

molar-volume and others added 8 commits July 27, 2020 21:35
2) test_permute_on_primitive_op implemented. It checks correctness and consistency of permute implementations.
2) added testcase for MatrixOp permute, to check consistency with  PauliOp.permute and CircuitOp.permute
@molar-volume
Copy link
Contributor Author

molar-volume commented Aug 3, 2020

Permute implemented for PauliOp and MatrixOp as well.

import numpy as np
from qiskit.aqua.operators import MatrixOp, X, Y, Z, I, CircuitOp

pauli_op = (X ^ Z).permute([0, 2]) @ (X ^ Y ^ Z ^ X)

matrix_op = (X ^ Z).to_matrix_op().permute([0, 2]) @ (X ^ Y ^ Z ^ X).to_matrix_op()

circuit_op = (X ^ Z).to_circuit_op().permute([0, 2]) @ (X ^ Y ^ Z ^ X).to_circuit_op()
print(circuit_op.to_circuit())

circ_eq_pauli = np.allclose(circuit_op.to_matrix(), pauli_op.to_matrix())  # circ_eq_pauli: True
circ_eq_matrix = np.allclose(circuit_op.to_matrix(), matrix_op.to_matrix())  # circ_eq_matrix: True

     ┌───┐┌───┐
q_0: ┤ X ├┤ I ├
     ├───┤├───┤
q_1: ┤ Z ├┤ Z ├
     ├───┤└───┘
q_2: ┤ Y ├─────
     ├───┤┌───┐
q_3: ┤ X ├┤ X ├
     └───┘└───┘

Do you want to provide compose with indices (or permute) for ListOps as well @Cryoris ?
The implementation of permute for ListOps might go in line with MatrixOp.permute

@woodsp-ibm
Copy link
Member

woodsp-ibm commented Aug 3, 2020

Is there are good reason for the rename of identity to identity_operator? Since this is a breaking change on any existing code, if we needed to do this, then we ought to be keep identity still (which would call over to the new function) but issue a deprecation warning with an appropriate message to use the method by its new name.

@molar-volume
Copy link
Contributor Author

@woodsp-ibm identity in OperatorBase was defined by me as part of this PR, I changed it to identity_operator, because @Cryoris pointed out that it is unclear what to expect from calling CircuitOp.identity

@molar-volume
Copy link
Contributor Author

molar-volume commented Aug 3, 2020

after reading #1086 I note that expand_to_dim will be overridden for SummedOp and ComposedOp, because e.g SummedOp.num_qubits is supposed to return the List[int]

@woodsp-ibm
Copy link
Member

Ah ok, identity is a new method that was added by this PR. so you just renamed your new method. From my perspective I am not sure that the name change does so much for me since they are all operators according to OperatorBase. Maybe the method anyway should be some class or even module level function to return a specific operator of said type i.e. an identity in this case. I do not know why it would be an instance method since it has no real need of self (or am I mistaken again) - unless it were an identity spanning the same number of qubits as the operator in self in which case I could somehow see it.

@molar-volume
Copy link
Contributor Author

molar-volume commented Aug 3, 2020

@woodsp-ibm self.identity_operator is called in method expand_to_dim, which is defined in OperatorBase.
So based on the self the specific implementation of identity_operator is called, see how it is used in PrimitiveOp._check_zero_for_composition_and_expand

@woodsp-ibm
Copy link
Member

Yes, I see some implementations of identity_operator do use self (when they need to copy across a property like is_measurement) while some do not. Do you see this as function that would be called in general by a user of the API - I guess you can if you want an instance that is an identity across that number of qubits - but of course with it being tied to self one needs to do that off an existing object which just seems weird. Otherwise its just a really some helper method to facilitate the one expand_to_dim implementation in the base class rather than have it implemented in each of the classes.

Copy link
Contributor

@Cryoris Cryoris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks for all the work!

@woodsp-ibm
Copy link
Member

I should add thanks for all your hard work and persistence to see this through. It has been a while, with a lot of discussion, so thanks again for your patience and perseverance in making this happen!

@woodsp-ibm woodsp-ibm merged commit 008337e into qiskit-community:master Sep 25, 2020
@molar-volume
Copy link
Contributor Author

My pleasure @woodsp-ibm ! I know the discussion was necessary and helped me a lot, so thanks for all your comments. Thanks @Cryoris !

@molar-volume molar-volume deleted the issue_1088 branch September 26, 2020 15:40
mtreinish pushed a commit to mtreinish/qiskit-core that referenced this pull request Nov 20, 2020
)

* 1) modified PrimitiveOp._check_zero_for_composition_and_expand, so that it expands shorter operator with identity
2) implemented OperatorBase.expand
3) defined abstract method OperatorBase.identity, and implemented in child classes of OperatorBase (TODO check if implementation makes sense for every class)
4) created tests for composing Operators of different dimensions

* CircuitStateFn.tensor did not set is_measurement parameter (hence always default to False), even if tensored instances were is_measurement=True

* Added test for expand method on StateFn subclasses

* 1) PauliOp.permute implemented
2) test_permute_on_primitive_op implemented. It checks correctness and consistency of permute implementations.

* 1) permute implemented for MatrixOp
2) added testcase for MatrixOp permute, to check consistency with  PauliOp.permute and CircuitOp.permute

* identity renamed to identity_operator in OperatorBase

* OperatorBase.expand renamed to expand_to_dim

* expand_to_dim overridden for PrimitiveOps

* 1) expand_to_dim implemented for each subclass of OperatorBase
2) removed identity_operator

* OperatorStateFn permute implemented

* 1) permute for TensoredOp
2) extended documentation

* 1) TensoredOp.permute moved to ListOp.permute
2) ListOp.permute uses CircuitOps instead of MatrixOps for permutations

* permute defined abstract in OperatorBase

* composition of PrimitiveOp with ComposedOp prepends the ComposedOp.oplist with PrimitiveOp

* Revert "composition of PrimitiveOp with ComposedOp prepends the ComposedOp.oplist with PrimitiveOp"

* 1) compose in MatrixOp, PauliOp and CircuitOp enhanced, to be consistent with ComposedOp.compose
2) test_compose_consistency added to verify the consistency

* DRY applied

* Test if ListOp.permute is consistent with PrimitiveOp permute methods

* test for expand_to_dim on ListOps

* 1) changed signature of compose to allow permutations on operators
2) permutation on operators included in compose method

* refactoring

* unit test for compose with indices

* refactoring and fixed linting

* 1) StateFn.compose expands the shorter operators with identity
2) refactored _check_zero_for_composition_and_expand
3) unit test for new composition features of StateFns
4) fixing style

* 1) expand_to_dim renamed to expand_with_identities (expand_to_dim was misleading, because its parameter is not the target dimension, but the number or qubits to add to operator)
2) documentation for the new features improved

* permute implemented for DictStateFn

* 1) VectorStateFn.to_dict_fn and DictStateFn.to_vector_state_fn implemented
2) VectorStateFn permute implemented
3) unit tests for the new functionality

* implemented custom function to decompose permutations into transpositions, to avoid import of sympy

* explaining comment for CircuitStateFn.expand_with_identity

* expand_with_identity made private and renamed to _expand_dim (expand_with_identity is misleading for implementations in DictStateFn and VectorStateFn, where zeros are used for expansion)

* 1) Compose method has only one set of permutation indices (for other operator).
2) Added new argument front=False in compose method. If front==True, return other.compose(self), but only after expansion and permutation is performed.

* 1) modified test_op_construction because of changes in compose signature
2) fix in CircuitOp.compose

* _check_zero_for_composition_and_expand renamed to _expand_shorter_operator_and_permute, to be more self-explanatory

* VectorStateFn.permute reimplemented for better performance

* removed duplicate method to_vector_state_fn (to_matrix_op was already defined)

* new_self instead of self in EvolvedOp

* test_op_construction refactored

* raise AquaError in ListOp.permute, if ListOp contains operators with different num_qubit

Co-authored-by: Manoel Marques <manoelmrqs@gmail.com>
Co-authored-by: Julien Gacon <gaconju@gmail.com>
Co-authored-by: Manoel Marques <Manoel.Marques@ibm.com>
Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
manoelmarques added a commit to manoelmarques/qiskit-terra that referenced this pull request Nov 23, 2020
)

* 1) modified PrimitiveOp._check_zero_for_composition_and_expand, so that it expands shorter operator with identity
2) implemented OperatorBase.expand
3) defined abstract method OperatorBase.identity, and implemented in child classes of OperatorBase (TODO check if implementation makes sense for every class)
4) created tests for composing Operators of different dimensions

* CircuitStateFn.tensor did not set is_measurement parameter (hence always default to False), even if tensored instances were is_measurement=True

* Added test for expand method on StateFn subclasses

* 1) PauliOp.permute implemented
2) test_permute_on_primitive_op implemented. It checks correctness and consistency of permute implementations.

* 1) permute implemented for MatrixOp
2) added testcase for MatrixOp permute, to check consistency with  PauliOp.permute and CircuitOp.permute

* identity renamed to identity_operator in OperatorBase

* OperatorBase.expand renamed to expand_to_dim

* expand_to_dim overridden for PrimitiveOps

* 1) expand_to_dim implemented for each subclass of OperatorBase
2) removed identity_operator

* OperatorStateFn permute implemented

* 1) permute for TensoredOp
2) extended documentation

* 1) TensoredOp.permute moved to ListOp.permute
2) ListOp.permute uses CircuitOps instead of MatrixOps for permutations

* permute defined abstract in OperatorBase

* composition of PrimitiveOp with ComposedOp prepends the ComposedOp.oplist with PrimitiveOp

* Revert "composition of PrimitiveOp with ComposedOp prepends the ComposedOp.oplist with PrimitiveOp"

* 1) compose in MatrixOp, PauliOp and CircuitOp enhanced, to be consistent with ComposedOp.compose
2) test_compose_consistency added to verify the consistency

* DRY applied

* Test if ListOp.permute is consistent with PrimitiveOp permute methods

* test for expand_to_dim on ListOps

* 1) changed signature of compose to allow permutations on operators
2) permutation on operators included in compose method

* refactoring

* unit test for compose with indices

* refactoring and fixed linting

* 1) StateFn.compose expands the shorter operators with identity
2) refactored _check_zero_for_composition_and_expand
3) unit test for new composition features of StateFns
4) fixing style

* 1) expand_to_dim renamed to expand_with_identities (expand_to_dim was misleading, because its parameter is not the target dimension, but the number or qubits to add to operator)
2) documentation for the new features improved

* permute implemented for DictStateFn

* 1) VectorStateFn.to_dict_fn and DictStateFn.to_vector_state_fn implemented
2) VectorStateFn permute implemented
3) unit tests for the new functionality

* implemented custom function to decompose permutations into transpositions, to avoid import of sympy

* explaining comment for CircuitStateFn.expand_with_identity

* expand_with_identity made private and renamed to _expand_dim (expand_with_identity is misleading for implementations in DictStateFn and VectorStateFn, where zeros are used for expansion)

* 1) Compose method has only one set of permutation indices (for other operator).
2) Added new argument front=False in compose method. If front==True, return other.compose(self), but only after expansion and permutation is performed.

* 1) modified test_op_construction because of changes in compose signature
2) fix in CircuitOp.compose

* _check_zero_for_composition_and_expand renamed to _expand_shorter_operator_and_permute, to be more self-explanatory

* VectorStateFn.permute reimplemented for better performance

* removed duplicate method to_vector_state_fn (to_matrix_op was already defined)

* new_self instead of self in EvolvedOp

* test_op_construction refactored

* raise AquaError in ListOp.permute, if ListOp contains operators with different num_qubit

Co-authored-by: Manoel Marques <manoelmrqs@gmail.com>
Co-authored-by: Julien Gacon <gaconju@gmail.com>
Co-authored-by: Manoel Marques <Manoel.Marques@ibm.com>
Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
manoelmarques added a commit to manoelmarques/qiskit-terra that referenced this pull request Dec 2, 2020
)

* 1) modified PrimitiveOp._check_zero_for_composition_and_expand, so that it expands shorter operator with identity
2) implemented OperatorBase.expand
3) defined abstract method OperatorBase.identity, and implemented in child classes of OperatorBase (TODO check if implementation makes sense for every class)
4) created tests for composing Operators of different dimensions

* CircuitStateFn.tensor did not set is_measurement parameter (hence always default to False), even if tensored instances were is_measurement=True

* Added test for expand method on StateFn subclasses

* 1) PauliOp.permute implemented
2) test_permute_on_primitive_op implemented. It checks correctness and consistency of permute implementations.

* 1) permute implemented for MatrixOp
2) added testcase for MatrixOp permute, to check consistency with  PauliOp.permute and CircuitOp.permute

* identity renamed to identity_operator in OperatorBase

* OperatorBase.expand renamed to expand_to_dim

* expand_to_dim overridden for PrimitiveOps

* 1) expand_to_dim implemented for each subclass of OperatorBase
2) removed identity_operator

* OperatorStateFn permute implemented

* 1) permute for TensoredOp
2) extended documentation

* 1) TensoredOp.permute moved to ListOp.permute
2) ListOp.permute uses CircuitOps instead of MatrixOps for permutations

* permute defined abstract in OperatorBase

* composition of PrimitiveOp with ComposedOp prepends the ComposedOp.oplist with PrimitiveOp

* Revert "composition of PrimitiveOp with ComposedOp prepends the ComposedOp.oplist with PrimitiveOp"

* 1) compose in MatrixOp, PauliOp and CircuitOp enhanced, to be consistent with ComposedOp.compose
2) test_compose_consistency added to verify the consistency

* DRY applied

* Test if ListOp.permute is consistent with PrimitiveOp permute methods

* test for expand_to_dim on ListOps

* 1) changed signature of compose to allow permutations on operators
2) permutation on operators included in compose method

* refactoring

* unit test for compose with indices

* refactoring and fixed linting

* 1) StateFn.compose expands the shorter operators with identity
2) refactored _check_zero_for_composition_and_expand
3) unit test for new composition features of StateFns
4) fixing style

* 1) expand_to_dim renamed to expand_with_identities (expand_to_dim was misleading, because its parameter is not the target dimension, but the number or qubits to add to operator)
2) documentation for the new features improved

* permute implemented for DictStateFn

* 1) VectorStateFn.to_dict_fn and DictStateFn.to_vector_state_fn implemented
2) VectorStateFn permute implemented
3) unit tests for the new functionality

* implemented custom function to decompose permutations into transpositions, to avoid import of sympy

* explaining comment for CircuitStateFn.expand_with_identity

* expand_with_identity made private and renamed to _expand_dim (expand_with_identity is misleading for implementations in DictStateFn and VectorStateFn, where zeros are used for expansion)

* 1) Compose method has only one set of permutation indices (for other operator).
2) Added new argument front=False in compose method. If front==True, return other.compose(self), but only after expansion and permutation is performed.

* 1) modified test_op_construction because of changes in compose signature
2) fix in CircuitOp.compose

* _check_zero_for_composition_and_expand renamed to _expand_shorter_operator_and_permute, to be more self-explanatory

* VectorStateFn.permute reimplemented for better performance

* removed duplicate method to_vector_state_fn (to_matrix_op was already defined)

* new_self instead of self in EvolvedOp

* test_op_construction refactored

* raise AquaError in ListOp.permute, if ListOp contains operators with different num_qubit

Co-authored-by: Manoel Marques <manoelmrqs@gmail.com>
Co-authored-by: Julien Gacon <gaconju@gmail.com>
Co-authored-by: Manoel Marques <Manoel.Marques@ibm.com>
Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
manoelmarques added a commit to manoelmarques/qiskit-terra that referenced this pull request Dec 7, 2020
)

* 1) modified PrimitiveOp._check_zero_for_composition_and_expand, so that it expands shorter operator with identity
2) implemented OperatorBase.expand
3) defined abstract method OperatorBase.identity, and implemented in child classes of OperatorBase (TODO check if implementation makes sense for every class)
4) created tests for composing Operators of different dimensions

* CircuitStateFn.tensor did not set is_measurement parameter (hence always default to False), even if tensored instances were is_measurement=True

* Added test for expand method on StateFn subclasses

* 1) PauliOp.permute implemented
2) test_permute_on_primitive_op implemented. It checks correctness and consistency of permute implementations.

* 1) permute implemented for MatrixOp
2) added testcase for MatrixOp permute, to check consistency with  PauliOp.permute and CircuitOp.permute

* identity renamed to identity_operator in OperatorBase

* OperatorBase.expand renamed to expand_to_dim

* expand_to_dim overridden for PrimitiveOps

* 1) expand_to_dim implemented for each subclass of OperatorBase
2) removed identity_operator

* OperatorStateFn permute implemented

* 1) permute for TensoredOp
2) extended documentation

* 1) TensoredOp.permute moved to ListOp.permute
2) ListOp.permute uses CircuitOps instead of MatrixOps for permutations

* permute defined abstract in OperatorBase

* composition of PrimitiveOp with ComposedOp prepends the ComposedOp.oplist with PrimitiveOp

* Revert "composition of PrimitiveOp with ComposedOp prepends the ComposedOp.oplist with PrimitiveOp"

* 1) compose in MatrixOp, PauliOp and CircuitOp enhanced, to be consistent with ComposedOp.compose
2) test_compose_consistency added to verify the consistency

* DRY applied

* Test if ListOp.permute is consistent with PrimitiveOp permute methods

* test for expand_to_dim on ListOps

* 1) changed signature of compose to allow permutations on operators
2) permutation on operators included in compose method

* refactoring

* unit test for compose with indices

* refactoring and fixed linting

* 1) StateFn.compose expands the shorter operators with identity
2) refactored _check_zero_for_composition_and_expand
3) unit test for new composition features of StateFns
4) fixing style

* 1) expand_to_dim renamed to expand_with_identities (expand_to_dim was misleading, because its parameter is not the target dimension, but the number or qubits to add to operator)
2) documentation for the new features improved

* permute implemented for DictStateFn

* 1) VectorStateFn.to_dict_fn and DictStateFn.to_vector_state_fn implemented
2) VectorStateFn permute implemented
3) unit tests for the new functionality

* implemented custom function to decompose permutations into transpositions, to avoid import of sympy

* explaining comment for CircuitStateFn.expand_with_identity

* expand_with_identity made private and renamed to _expand_dim (expand_with_identity is misleading for implementations in DictStateFn and VectorStateFn, where zeros are used for expansion)

* 1) Compose method has only one set of permutation indices (for other operator).
2) Added new argument front=False in compose method. If front==True, return other.compose(self), but only after expansion and permutation is performed.

* 1) modified test_op_construction because of changes in compose signature
2) fix in CircuitOp.compose

* _check_zero_for_composition_and_expand renamed to _expand_shorter_operator_and_permute, to be more self-explanatory

* VectorStateFn.permute reimplemented for better performance

* removed duplicate method to_vector_state_fn (to_matrix_op was already defined)

* new_self instead of self in EvolvedOp

* test_op_construction refactored

* raise AquaError in ListOp.permute, if ListOp contains operators with different num_qubit

Co-authored-by: Manoel Marques <manoelmrqs@gmail.com>
Co-authored-by: Julien Gacon <gaconju@gmail.com>
Co-authored-by: Manoel Marques <Manoel.Marques@ibm.com>
Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Changelog: New Feature Include in the Added section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CircuitStateFn.tensor does not set is_measurement parameter Operator.compose should allow indices
4 participants