Fixing python passes and docstrings related to approximation_degree#16006
Fixing python passes and docstrings related to approximation_degree#16006alexanderivrii wants to merge 1 commit intoQiskit:mainfrom
Conversation
|
One or more of the following people are relevant to this code:
|
Cryoris
left a comment
There was a problem hiding this comment.
This is more of a docs cleanup, since it doesn't really add new explanations, does it? 🙂
| """ | ||
|
|
||
| def __init__(self, approximation_degree: float = 1.0): | ||
| def __init__(self, approximation_degree: float | None = 1.0): |
There was a problem hiding this comment.
If None just immediately defaults to 1.0, why would we change to supporting None as input? The current interface seems complete without it, no? (Same question for CommutativeOptimization & co)
There was a problem hiding this comment.
I am not sure I fully understand the question. Calling CommutativeOptimization(approximation_degree=None) is not the same as calling CommutativeOptimization(), as in the former case approximation_degree will be None, and in the latter case it will the default value of 1.0, do you agree?
In other transpiler passes, passing approximation_degree=None means "compute approximation degree based on the target", so this option has a special meaning that I want to expose here as well. This will also allow a more consistent story: in all user-facing functions (Python and C) approximation_degree is of type float | None.
Having said the above, CommutativeOptimization currently does not take target as an argument (so internally we do set approximation_degree to 1.0), however we want to change this in the future.
There was a problem hiding this comment.
I'm saying that CommutativeOptimization(approximation_degree=None) is not a valid input, so we don't have to support it. If we add a Target support in the future we can enable setting it to None -- or is there a reason to add it now?
There was a problem hiding this comment.
I can remove it. But I think it would be nice to have the same convention for what approximation degree is across the code.
There was a problem hiding this comment.
I agree it would be nice, but for that we'd need to have the Target-specific behavior implemented 🙂
There was a problem hiding this comment.
Note that we already have a similar workaround in ConsolidateBlocks:
More importantly, since we allow approximation_degree to be None when generating preset pass managers, defining plugins, etc., we need to explicitly remember to overwrite None with 1.0 when calling CommutativeOptimization and Substitute4PiRotations, or else we will hit python type errors. The only reason we don't get these errors now is because we currently only enable these passes in the Clifford+T pipeline and because I forgot to pass the approximation_degree to these passes 😅.
This PR aims to slightly improve documentation related to
approximation_degree, and in particular whether it is allowed to beNone. For instance, ingenerate_preset_pass_manager,approximation_degreewas previously documented asfloat, and yet the value ofNoneis supported, meaning that it should be computed based onTarget. So far in this PR I only touched the docstrings that explicitly mentionapproximation_degree, but I can also expand it to more docstrings if needed.There was also a minor inconsistency with how various transpiler passes interpret
approximation_degree: the far majority of passes allow bothfloatandNone, butCommutativeOptimizationandSubstitutePi4Rotationspreviously only supportedfloat. This PR also trivially extends these two passes to supportNone. I don't know if this should be considered a new feature, but I can add a release note if required.AI/LLM disclosure