Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement the QubitMappers #18

Closed
14 tasks done
manoelmarques opened this issue Jan 28, 2021 · 13 comments
Closed
14 tasks done

Implement the QubitMappers #18

manoelmarques opened this issue Jan 28, 2021 · 13 comments

Comments

@manoelmarques
Copy link
Contributor

manoelmarques commented Jan 28, 2021

Migrated from Enterprise Github.
Creator: @mrossinek

The state of the QubitMappers is the following:

  • Implementation
    • JordanWignerMapper (fermionic)
    • ParityMapper (fermionic)
    • BravyiKitaevMapper (fermionic)
    • BravyiKitaevSuperFastMapper (fermionic)
    • DirectMapper (bosonic)
    • LinearMapper (spin)
    • LogarithmicMapper (spin)
  • Unittests
    • JordanWignerMapper (fermionic)
    • ParityMapper (fermionic)
    • BravyiKitaevMapper (fermionic)
    • BravyiKitaevSuperFastMapper (fermionic)
    • DirectMapper (bosonic)
    • LinearMapper (spin)
    • LogarithmicMapper (spin)

Note: this issue may be broken down into several issues as work starts to progress.

@manoelmarques
Copy link
Contributor Author

Creator: @mrossinek

You can find an example of the new Jordan-Wigner mapping in action here:
https://gist.github.ibm.com/oss-zurich/a2b75f58d4cd2eae7df8e1f1e9dcd6b0

@manoelmarques
Copy link
Contributor Author

Creator: @woodsp-ibm

Can we look at avoiding isinstance and using methods implemented on a base class/interface to do things like this. If someone wants to add another operator that is of type Fermionic then an instance check like this is problematic. There is already some type field that seems to be passed to superclass - can we not have some method query based off that, or an extension thereof, in some way.

        if isinstance(particle_type, FermionicOperator):
            return True
        return False

@manoelmarques
Copy link
Contributor Author

Creator: @pbark

@woodsp I think that the idea behind this is that not all mappings work with all type of operators. e.g. if one provides a BosonicOperator in the JW mapping this is not going to work. Sure we can try to think ways of spliting the mappings (e.g. FermionicMappings VS BosonicMappings) but I am not sure how much complicated this can be.

@manoelmarques
Copy link
Contributor Author

Creator: @mrossinek

I partially agree with @woodsp here. The isinstance call is not the nicest solution here but I went for it because it was the quickest route to a prototype.
Could you explain what you mean by

There is already some type field that seems to be passed to superclass - can we not have some method query based off that, or an extension thereof, in some way.

I am not sure which type field you are referring to, here. But I am open to suggestions on how to improve this! Feel free to propose a more concrete example or open a PR directly 👍

EDIT: I didn't see @bpa comment before posting. You are absolutely right too! We definitely do need some mechanism for checking that the type matches but I think @woodsp is proposing a solution which circumvents the isinstance call. That would be great because (as he also says in his example) this leaves more room for future expandability.

@manoelmarques
Copy link
Contributor Author

Creator: @woodsp-ibm

I think I was looking at this, since its in the Mapping base class, where it seemed there was already some type notion, and where ParticleOperator seemed to define the type via a string if I recall.

    def supports_particle_type(self, particle_type: ParticleOperator) -> bool:

Whether this is useful or not it would be nice to avoid isinstance checks however that is done.

@manoelmarques
Copy link
Contributor Author

Creator: @mrossinek

That's a good point! Both, the ParticleOperator and SumOp have a particle_type-attribute which is a string (see e.g. here). That should be pretty well suited for our needs here.

In either case, we would have to update this function call anyways because now that we split the primitives out the SumOp-kinds no longer implement the ParticleOperator interface. So the above would actually have become:

    def supports_particle_type(self, particle_type: Union[ParticleOperator, SumOp]) -> bool:

Instead, we can now just do:

    def supports_particle_type(self, particle_type: str) -> bool:

and use the particle_type attribute of each operator as the input for this method.

@mrossinek
Copy link
Member

We would like to remove all of the mapper logic from the operators in order to cleanly separate this module. One motivation is that in the future we may not need to map these operators for all possible backends which we may want to use.
This means, that the mappers need to implement the translation logic from a ParticleOp.label to a (sum of) Pauli string(s) themselves. Rather than relying on the old pauli_table style, we can rely directly on a conversion logic which understands the relation of the ParticleOp.label-alphabet in the same way in which the mappings are defined.

To make this more concrete with an example, the JordanWigner mapper would translate in the following style:

'+III' -> 0.5 * ('IIIX' + i * 'IIIY')
'I+II' -> 0.5 * ('IIXZ' + i * 'IIYZ')
...

(respecting the qubit ordering in Qiskit)
This logic can be generalized in functional form given the definitions of the JW mapping (see literature).

The BKSF mapping can also be implemented in such a style as the edge list can be inferred from fermionic labels containing + and - characters. As long as a consistent approach is being followed in determining the edge list from a given label, e.g.:

'++--' -> (0, 3, 1, 2)

where the above example means that 0 -> 3 and 1 -> 2 are the edges in this list. By always pairing + and - pairs in (e.g. inner-to-outer) style a consistent mapping should take place. The sign of the coefficient will already be taken care of when the FermionicOp gets constructed from the initial list of integrals.

@mrossinek
Copy link
Member

screenshot_1612460491

@woodsp-ibm
Copy link
Member

woodsp-ibm commented Feb 4, 2021

I am wondering whether there should just be FermionicMapper, BosonicMapper and SpinMapper base classes with a map taking their respective Operator and returning a PauliSumOp. Mapping a mixed 2ndQOp requires doing the mappings against each of the types in there - I am struggling to see where bringing this to that level of a SecondQuantizedQubitMapper abstraction is of benefit. As a parallel to 2ndQOp would a 2ndQOpMapper not be expected to contain a FermonicMapper, a BosonicMapper and SpinMapper in a similar way that the 2ndQOperator contains one of each operator types should such a class exist?

@mrossinek
Copy link
Member

Yes I think FermionicMapper, etc. make sense. I will update the UML again later.
And what you suggest above would then be similar to what we previously thought of as a MixedMapper:

class MixedMapper():
    def __init__(self, fermionic, bosonic, spin):
        ...

    def map(self, second_q_op):
        fermionic_paul_op = self.fermionic.map(second_q_op.fermionic)
        ...

Obviously the name can be made whatever we like 👍

@mrossinek mrossinek changed the title Implement the QubitMappings Implement the QubitMappers Mar 12, 2021
@mrossinek
Copy link
Member

All mappers which will be part of the upcoming release are finished. The remaining ones will be tracked in their own issues:

Thus, I am closing this broader issue in favor of these other ones.

@pbark
Copy link
Contributor

pbark commented Apr 21, 2021

Reopening the issue for planning and making it an Epic

@pbark pbark reopened this Apr 21, 2021
@pbark pbark added the Epic label Apr 21, 2021
@mrossinek
Copy link
Member

The final mapper tracked by this Epic was just merged into main from #624

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants