Integration into the current PL device ecosys #6684
+1,665
−6,032
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ultMixed In this commit, we would like to replace all the usecase across the whole codebase that require the legacy default mixed from DefaultMixed to DefaultMixedLegacy
Co-authored-by: Astral Cai <astral.cai@xanadu.ai>
|
Finally we passed the CodeCov 🥳 |
PietropaoloFrisoni
approved these changes
Feb 28, 2025
|
3450f5b patched the missing coverage of device constructor. Since its design is to process device name, we are not able to make use of LegacyQubitDevice. Instead, we do a dumb construction with custom decomposition (i.e. no actual call of expand, which might lead to inf recursion) for |
albi3ro
reviewed
Feb 28, 2025
albi3ro
reviewed
Feb 28, 2025
albi3ro
reviewed
Feb 28, 2025
Co-authored-by: Christina Lee <christina@xanadu.ai>
JerryChen97
added
the
ci:run-full-test-suite
mudit2812
reviewed
Mar 10, 2025
mudit2812
reviewed
Mar 10, 2025
Comment on lines
+603
to
+604
| for op_class in SYMMETRIC_REAL_OPS: | ||
| apply_operation.register(op_class)(apply_symmetric_real_op) |
There was a problem hiding this comment.
Can you leave a comment about why this syntax was used rather than using @apply_operation.register` as a decorator? It was so that the docs render nicely right?
tests/devices/test_default_mixed.py
Outdated
Comment on lines
104
to
137
| class TestLegacyDefaultMixed: | ||
| """ | ||
| Tests that covered parts of legacy method of the DefaultMixed device. | ||
| """ | ||
|
|
||
| @pytest.mark.all_interfaces | ||
| @pytest.mark.parametrize("interface", ML_INTERFACES) | ||
| @pytest.mark.parametrize( | ||
| "total_wires", [qml.wires.Wires([0]), qml.wires.Wires([0, 1]), qml.wires.Wires([0, 2, 1])] | ||
| ) | ||
| def test_apply_state_vector(self, total_wires, interface): | ||
| """Initialize the internal state in a specified pure state.""" | ||
| if interface == "autograd": | ||
| pytest.skip( | ||
| "Autograd interface not supported for this test. No autograd scatter support." | ||
| ) | ||
| num_total_wires = len(total_wires) | ||
|
|
||
| device_wires = qml.wires.Wires([0]) | ||
| num_wires = len(device_wires) | ||
| tolerance = 1e-10 | ||
| state_np = np.zeros(shape=(2**num_wires,), dtype=np.complex128) | ||
| state_np[0] = 1.0 | ||
| _state = _apply_state_vector( | ||
| total_wires=total_wires, | ||
| state=state_np, | ||
| device_wires=device_wires, | ||
| interface=interface, | ||
| ) | ||
|
|
||
| rho_expected_numpy = np.zeros([2] * 2 * num_total_wires, dtype=np.complex128) | ||
| rho_expected_numpy[tuple([0] * 2 * num_total_wires)] = 1.0 | ||
| rho_expected = qml.math.asarray(rho_expected_numpy, like=interface) | ||
| assert qml.math.allclose(_state, rho_expected, atol=tolerance) |
There was a problem hiding this comment.
I think it's better to remove _apply_state_vector and just add a unit test for scatter.
There was a problem hiding this comment.
Alright! I'll try cleaning it up here
There was a problem hiding this comment.
@mudit2812 Redesigned the unitttests here ce0e8ad
JerryChen97
commented
Mar 10, 2025
JerryChen97
removed
the
ci:run-full-test-suite
mudit2812
approved these changes
Mar 10, 2025
| @@ -600,6 +600,7 @@ def apply_symmetric_real_op( | |||
| return state | |||
|
|
|||
|
|
|||
| # NOTE: this loop is for a nice doc rendering for `apply_operation`. With a direct multiple single registers over different op class there will be severe rendering issue as discussed in https://github.com/PennyLaneAI/pennylane/pull/6684#pullrequestreview-2565634328 | |||
There was a problem hiding this comment.
Just splitting it into multiple lines for readability
Suggested change
| # NOTE: this loop is for a nice doc rendering for `apply_operation`. With a direct multiple single registers over different op class there will be severe rendering issue as discussed in https://github.com/PennyLaneAI/pennylane/pull/6684#pullrequestreview-2565634328 | |
| # NOTE: this loop is for a nice doc rendering for `apply_operation`. With a direct multiple single | |
| # registers over different op class there will be severe rendering issue as discussed in | |
| # https://github.com/PennyLaneAI/pennylane/pull/6684#pullrequestreview-2565634328 |
|
We are finally merging this PR! 🍾 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context:
We finished all the necessary components of the new API interface of
default_mixed. However, since this is an internal change, which involves many different files and paths, we would like to have one more round of check, success and merge of which should concludes the whole epic.Description of the Change:
keep DefaultMixed, but rename it to DefaultMixedLegacy (note: still the legacy one will be purged before this PR merged into master; it's kept right now due to great help in debug)operations_mixedback tooperations, and copy it to DefaultMixedLegacyreadout_errorjust aswith arbitrary input error channelsBitFlipmodify all the device tests inDeleted legacy specified device tests. These should be covered in the integration test suite mentioned abovetests/devicesaccordingly. Basically thetests/devices/test_default_mixed*.pyif no errors come up, then separate the legacy code into another fileLots of errors, see drawback section for a record along with updates.test_return_types_qnodedefault.mixeddev._stateneeds to be removed and may be replaced by equivalent or not. Sol: had a PR to improve this linepip install git+https://github.com/PennyLaneAI/pennylane-qiskit.git@master)why user warning here? Didn't read like sensible. Should double check.when running on notebook everything is finesome by-the-way improvement:
get_canonical_interface_nameand removed the hardcoded dictionary mimic of enum.qubit_mixedmuch more withqubitmoduleBenefits:
Legacy code was preserved for reference purpose as well as catering for potential usersBetter separation between new interface and legacydefault.mixedis ported directly with our new API.Possible Drawbacks:
default_mixedmight find it annoying for a while. However, we would include a direct indication in changelog as well as the new class.unexpected bug 1:finite-diffsupport forPurityMPon torch, tf, and jax downunexpected bug 2: gradient calculation atPhaseFlip(0)breaks withnanreturned at initial state |0> (all good for other other channels or other state with PhaseFlip`expected bug 3: classical shadow is down for most of tests. This is due to the fact that the implementation is hidden insideQubitDevicewhich was missed out before. Need to be fixed asapThe drop of legacy device might introduce some missing in related code coverage. For visibility, we track them as todos here as well.
ClassicalShadowrelated missing: the entireClassicalShadowMP.process()method;shadow_expvalbranch in_qubit_device.pysingle_dispatchandmutliple_dispatch)scatterrelated missing:this is suspicious since it might imply that some functinoalities were lost.nothing has been lost. The legacy code that usescatterwas for initializing states in a bruteforce way, which is not needed anymore.Related GitHub Issues:
[sc-73324]