Skip to content

Conversation

akissinger
Copy link
Member

Currently the way PyZX handles scalars is not compatible with the Rust backend. The reason for this is Rust stores its scalars differently, so asking for the scalar from the python bindings returns a (lossy) copy of the current scalar. Hence, code like this which mutates the scalar doesn't work as expected:

g.scalar.add_power(5)  # no-op for the Rust backend

I suggest we deprecate mutating the scalar directly, and do this all via helper methods added to BaseGraph, which are added by this PR:

def mult_scalar_by_phase(self, phase: FractionLike) -> None:
    """Multiplies the scalar by a phase factor."""
    self.scalar.add_phase(phase)

def mult_scalar_by_sqrt2_power(self, power: int) -> None:
    """Multiplies the scalar by sqrt(2) raised to the given power."""
    self.scalar.add_power(power)

def mult_scalar_by_scalar(self, scalar: Scalar) -> None:
    """Multiplies scalar with the given scalar"""
    self.scalar.mult_with_scalar(scalar)

def mult_scalar_by_spider_pair(self, phase1: FractionLike, phase2: FractionLike) -> None:
    """Multiplies scalar with a 'spider pair', i.e. a pair of phased Z-spiders connected by an H edge"""
    self.scalar.add_spider_pair(phase1, phase2)

This lets the Rust backend override these methods to do the correct thing.

I will probably go ahead and merge this soon and also change all of the instances I find of mutating the scalar, since this shouldn't break anything. However, on the long term, we might want to look at e.g. renaming Basegraph.scalar to Basegraph._scalar to enforce using the helpers.

@akissinger
Copy link
Member Author

@lara-madison and @jvdwetering can I go ahead and merge this? Hopefully it doesn't clobber anything else you are working on at the moment.

@jvdwetering
Copy link
Collaborator

That seems reasonable to me. I also agree that these names make more sense. We could also change the names of the corresponding method of the Scalar class, but that doesn't have to be in this PR.
I think this doesn't break anything on our end so you could go ahead and merge.

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

Successfully merging this pull request may close these issues.

2 participants