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

Add typing.Result and typing.ResultBatch type variables #4108

Merged
merged 15 commits into from May 16, 2023

Conversation

albi3ro
Copy link
Contributor

@albi3ro albi3ro commented May 9, 2023

The results of an execution are difficult to concretely type hint due to the nested and dynamic shape of the object. Nonetheless, type hints are essential for clearly specific an interface and communicating what a function or method does.

pennylane.typing.Result, a typing.TypeVar specifies the result of a single execution. pennylane.typing.ResultBatch, a tuple of Result, specifies the result of a batch of executions.

I would prefer it if we could add documentation to a TypeVar, but at least this is an improvement and solves our current problem.

This PR already adds the type hint to a couple of key places, mostly for the new device interface. As time goes on, we can improve coverage of type hints.

@albi3ro albi3ro requested review from rmoyard and mudit2812 May 9, 2023 18:07
@codecov
Copy link

codecov bot commented May 9, 2023

Codecov Report

Merging #4108 (834a028) into master (5a36d6f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #4108   +/-   ##
=======================================
  Coverage   99.76%   99.76%           
=======================================
  Files         345      345           
  Lines       30920    30934   +14     
=======================================
+ Hits        30848    30862   +14     
  Misses         72       72           
Impacted Files Coverage Δ
pennylane/devices/experimental/default_qubit_2.py 100.00% <100.00%> (ø)
pennylane/devices/experimental/device_api.py 98.00% <100.00%> (+0.12%) ⬆️
pennylane/devices/qubit/preprocess.py 100.00% <100.00%> (ø)
pennylane/devices/qubit/simulate.py 100.00% <100.00%> (ø)
pennylane/interfaces/execution.py 100.00% <100.00%> (ø)
pennylane/qnode.py 100.00% <100.00%> (ø)
pennylane/transforms/batch_transform.py 100.00% <100.00%> (ø)
pennylane/transforms/broadcast_expand.py 100.00% <100.00%> (ø)
pennylane/typing.py 100.00% <100.00%> (ø)

Copy link
Contributor

@rmoyard rmoyard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, it looks good! A couple of minor comments

pennylane/devices/experimental/device_api.py Show resolved Hide resolved
pennylane/devices/experimental/default_qubit_2.py Outdated Show resolved Hide resolved
pennylane/devices/experimental/default_qubit_2.py Outdated Show resolved Hide resolved
pennylane/interfaces/execution.py Outdated Show resolved Hide resolved
Copy link
Contributor

@mudit2812 mudit2812 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Christina, this is coming along well. A few type hints need to be fixed, and I agree with @rmoyard 's suggestion about importing Result and ResultBatch from qml.typing. rather than using qml.typing.Result or qml.typing.ResultBatch.

pennylane/devices/experimental/default_qubit_2.py Outdated Show resolved Hide resolved
pennylane/devices/experimental/device_api.py Outdated Show resolved Hide resolved
pennylane/devices/qubit/preprocess.py Outdated Show resolved Hide resolved
pennylane/devices/qubit/preprocess.py Outdated Show resolved Hide resolved
pennylane/devices/qubit/simulate.py Outdated Show resolved Hide resolved
pennylane/transforms/batch_transform.py Outdated Show resolved Hide resolved
@albi3ro albi3ro requested review from mudit2812 and rmoyard May 16, 2023 16:06
Copy link
Contributor

@rmoyard rmoyard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

pennylane/qnode.py Outdated Show resolved Hide resolved
albi3ro and others added 2 commits May 16, 2023 14:38
@albi3ro albi3ro enabled auto-merge (squash) May 16, 2023 18:39
@albi3ro albi3ro merged commit 2f89be4 into master May 16, 2023
43 checks passed
@albi3ro albi3ro deleted the result-type-hint branch May 16, 2023 19:13
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.

None yet

3 participants