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
363 stars 160 forks source link

Return type of RichardsonFactory #651

Closed mstechly closed 3 years ago

mstechly commented 3 years ago

Pre-Report Checklist

Issue Description

The return type of extrapolate in RichardsonFactory is a non-obvious Union, which seems to be a bad solution for a couple of reasons:

  1. It's confusing to the user.
  2. It might make it harder to write modular code, as some functions might depend on one or the other type of the output, which might cause confusion or ugly bugs
  3. It might not work well with static type checking (though I haven't checked it properly!)

One solution that I can think of is having two functions, e.g. extrapolate and extrapolate_with_full_output? Any other ideas @dexter2206 ?

Code Snippet

Union[
        float,
        Tuple[
            float,
            Union[float, None],
            List[float],
            Union[np.ndarray, None],
            Callable[[float], float],
        ],
github-actions[bot] commented 3 years ago

Hello @mstechly, thank you for your interest in Mitiq! If this is a bug report, please provide screenshots and/or minimum viable code to reproduce your issue, so we can do our best to help get it fixed. If you have any questions in the meantime, you can also ask us on the Unitary Fund Discord.

andreamari commented 3 years ago

Thanks @mstechly! We want Mitiq to be as user friendly as possible and so this kind of constructive criticism and feedback is very useful.

I write down some proposals:

  1. We could have two methods extrapolate and extrapolate_with_full_output (suggested by @mstechly).
  2. We could always return the full output and so get rid of the full_output option.
  3. Moreover we could also simplify the output to Tuple[float, Dict[str, Any]], where the float is the zne value and the dictionary contains all the rest. See e.g. this function https://github.com/unitaryfund/mitiq/blob/master/mitiq/pec/pec.py#L40.
  4. Alternatively, perhaps too drastically, we could always return just Dict[str, Any]] where the zne value is one of the elements.

Personally, I lean towards 2 + 3. But other opinions are welcome.

rmlarose commented 3 years ago

What about a custom return type?

class WhateverFullOutputReturns:
   ...

class Factory:
    ...
    def extrapolate(...) -> Union[float, WhateverFullOutputReturns]:

Or use the custom return type in all cases?

class ExtrapolateOutput:
    ...

class Factory:
    ...
    def extrapolate(...) -> ExtrapolateOutput:
mstechly commented 3 years ago

@rmlarose I like this idea, perhaps ExtrapolateOutput could be a NamedTuple? It could still use the flag to avoid calculating all the additional quantities if they're not needed. They would be just set to None if the flag is not specified?

rmlarose commented 3 years ago

@rmlarose I like this idea, perhaps ExtrapolateOutput could be a NamedTuple?

Yeah, something like this. Whatever minimizes the annotation while not hurting usability is fine by me.

mstechly commented 3 years ago

Also worth pointing out that this will break backward compatibility.

rmlarose commented 3 years ago

Discussed at Mitiq meeting, strong preference for this option.

class ExtrapolateOutput:
    ...

class Factory:
    ...
    def extrapolate(...) -> ExtrapolateOutput: