unitaryfund / mitiq

Mitiq is an open source toolkit for implementing error mitigation techniques on most current intermediate-scale quantum computers.
https://mitiq.readthedocs.io
GNU General Public License v3.0
368 stars 163 forks source link

Better type for `QPROGRAM` #746

Open rmlarose opened 3 years ago

rmlarose commented 3 years ago

Currently QPROGRAM is either a typing.Union of several quantum circuit types, or a single type if only Cirq is installed. It should be consistent in both cases - it was a bit of a pain to write a simple test in #745. Further, the typing.Union type is not particularly useful, notably for isinstance checks.

Desired behavior

anushkrishnav commented 3 years ago

I have some knowledge on types can give it a try , can you share more resources/examples or starters?

github-actions[bot] commented 2 years ago

This issue had no activity for 2 months, and will be closed in one week unless there is new activity. Cheers!

github-actions[bot] commented 2 years ago

This issue had no activity for 2 months, and will be closed in one week unless there is new activity. Cheers!

github-actions[bot] commented 2 years ago

This issue had no activity for 4 months, and will be closed in 2 weeks unless there is new activity. Cheers!

nathanshammah commented 8 months ago

@cosenal great you are working on this, @FarLab I'm adding it to the current milestone.

cosenal commented 7 months ago

@rmlarose Sorry for necromancing this after many years :D

I understand how QPROGRAM collapses to the type Circuit when no other circuit types are installed. We could add a dummy type in the Union to avoid this.

However, QPROGRAM should only be used for type hinting. When do you envision Mitiq users/maintainers to inspect QPROGRAM at run-time? The only use cases I can think of are similar to the one in the test you implemented in #745, but those are "meta" use-case that in turn are due to the way we implement those imports.

cosenal commented 7 months ago

@natestemen A friendly reminder to explain your concern about the use of a union type here (I remember you had something to say about this at the last community call).

natestemen commented 7 months ago

Thanks for the ping, and apologies for the delay!

My main concern is related to how the definition of QPROGRAM affects the capability of defining good typehints for executors. Right now, the simplest thing to do is Callable[[QPROGRAM], float]. This allows for somewhat strange executors to pass the typecheck such as the following.

def execute(circuit: Union[qiskit.QuantumCircuit, pyquil.Program]) -> float:
    ...

Ideally, we should still be able to use Callable[[QPROGRAM], float] since it is succinct and gets the idea across, but only functions which accept a single QPROGRAM type should pass. This could be achieved with something like this typehint.[^1]

ExecutorLike = Union[
    Callable[[_Circuit], float],
    Callable[[_Program], float],
    Callable[[_QuantumCircuit], float],
    Callable[[_BKCircuit], float],
    Callable[[_QuantumTape], float],
    Callable[[_QiboCircuit], float],
]

As to whether it's worth putting time in to achieve this goal, I'm not totally sure.

I asked a question related to this on stackoverflow and got the recommendation to do something like this.

T = TypeVar("T", bound=QPROGRAM)

Last I tried this, I got lots of mypy errors which were not easy/straightforward to fix.


Anyway, that's the background that I never wrote down. Thanks for prompting me to. We definitely don't need to continue down this path, but wanted to air my difficulties that I had when I found out the type hints are not perfect.

[^1]: But then this doesn't account for the fact that some executors are batched, meaning they can take a sequence of quantum circuits, instead of just one.

cosenal commented 7 months ago

Thanks, @natestemen! I understand the concern, and I think it's orthogonal to the OP's concern, which was about insepctability of the type.

First, I don't agree that an executor with a signature

def execute(circuit: Union[qiskit.QuantumCircuit, pyquil.Program]) -> float:

is a strange executor. In fact, given the current Mitiq design, I think we should leave the flexibility to user of creating executor functions that may take any combination of the program types allowed. What is important is that in error mitigation routines (e.g. execute_with_zne), the circuit and the input argument of the executor function have the same type. In that sense, the stackoverflow's answer of creating a typevar that bounds to QPROGRAM is a good solution.

In order to understand the concern better, I took our gold snippet from the README, and ran mypy against it. It turned out to be an interesting exercise. Come onto this journey with me on the video! 🍿

https://github.com/unitaryfund/mitiq/assets/1048336/acfb6e2a-c252-4812-a3eb-f52d8ea7fd25

natestemen commented 7 months ago

I think it's orthogonal to the OP's concern

You're right, and I agree that increasing the inspectability of QPROGRAM isn't what we should strive for here. I'm not sure a Mitiq user ever needs to know about QPROGRAM, actually. Since the title of this ticket is "Better type for QPROGRAM", lets just keep this ticket as the one tracking improvements here, unless you feel strongly otherwise.

I don't agree that an executor with a signature ... is a strange executor

Hmmm, well I still think it is a little bit, but I agree with you that we should allow it. If that's what the user wants, let them have it :)

What is important is that [...] the circuit and the input argument of the executor function have the same type

Yes! Thank you for figuring out what my problem was in a much cleaner, and elegant way than I was able to articulate.

your video

First, of all, I really enjoyed the nice walk-through you did here. It was really clear, easy to follow, and informative. I think the idea of using videos where needed is actually a great idea, and something we should be aware of.

Second, I just wanted to point out that the snippet in the README does not have typing because we do not expect users to use typing. I think we should try to keep it simple, and understandable for all python coders (some might get confused by the type hints). Either way, I think what you went through should work with mypy since we want the savvy user to be able to use the tool if they so choose.

Final thoughts

Nice investigation, and clear demonstration. I think you've mentioned in one of the calls, but moving QPROGRAM from a union to TypeVar seems like a fix. As you've probably seen it's a decently deep-reaching change. Happy to help if needed.

cosenal commented 7 months ago

I agree with keeping this ticket open for tracking improvements on how the type QPROGRAM is used throughout the codebase 💪🏻

I am glad you enjoyed the video ☺️ Let me clarify that I didn't mean that the snippet in the README should have typing. I have just used it as a very simple and self-contained example of code that uses mitiq API and for which there should be a way to type it. Surprispingly, as demonstrated by the video, there is no straightforward way to type it, which implies the current Mitiq typing system is not coherent, which, in turns, implies that the API is not as clean as it should be.

In any case, we agree on the solution, that is, using a TypeVar bounded to QPROGRAM in all those function arguments that are typed as QPROGRAM now. I will work on this solution and I will try to make the changes as incremental as possible.