feat: Add platonic universal dynamic decoupling sequence for qudits#334
feat: Add platonic universal dynamic decoupling sequence for qudits#334hectorBrown wants to merge 5 commits intoqctrl:masterfrom
Conversation
|
Hi, quick update: I’m currently based here in Sydney and have applied for the Software Engineering role with the Quantum Sensing team. I’m really keen on all the work you are doing, and would love to hear any thoughts you have on this PR or any specific hardware-interfacing challenges the team is currently tackling! |
There was a problem hiding this comment.
Thanks for the contribution @hectorBrown !
Unfortunately, myself and the other maintainers of this package are not in the Sensing division, so I have little to share about their hardware challenges.
| assert _pulses_produce_identity(xy_concat_sequence) | ||
|
|
||
|
|
||
| def test_platonic_sequence(): |
There was a problem hiding this comment.
This test ends up being very long and convoluted. If it fails at any point it'd be hard to assess from the failure e.g. for which order it broke. I would suggest splitting it into separate tests for the different orders. (We could also use something like pytest.mark.parametrize, but I think the code would end up still being quite convoluted with so many options, which we don't want in simple unit tests.)
|
|
||
| # The generators assocaited with each point group. | ||
| phi = (np.sqrt(5) + 1) / 2 # golden ratio | ||
| generators = { |
There was a problem hiding this comment.
Rather than defining these generators and eulerian_paths dictionaries, can we usematch/case or an if-elif chain and directly build the arrays in the different branches? It would save some memory and make the code more readable. There is no advantage in having an object that holds all the information at once in the implementaiton.
| eulerian_paths = { | ||
| "Dihedral": [0, 1, 0, 1, 1, 0, 1, 0], | ||
| "Tetrahedral": [ 0, 1, 0, 0, 1, 0, 1, 1, 1, 0, 0, 1, 0, 1, 1, 1, 0, 0, 1, 0, 1, 1, 0, 0], | ||
| "Octahedral": [ 0, 1, 0, 0, 0, 1, 1, 1, 0, 1, 0, 0, 1, 1, 1, 0, 0, 1, 0, 1, 1, 0, 0, 0, 0, 1, 0, 1, 1, 1, 0, 1, 0, 0, 1, 1, 0, 0, 0, 0, 1, 0, 1, 1, 1, 0, 1, 1 ], | ||
| "Icosahedral": [ 1, 0, 0, 0, 1, 1, 0, 0, 1, 0, 0, 0, 0, 0, 1, 1, 0, 0, 0, 1, 0, 1, 1, 1, 0, 1, 0, 0, 1, 1, 0, 0, 1, 1, 0, 1, 1, 0, 1, 1, 0, 1, 1, 1, 0, 0, 0, 0, 1, 0, 1, 1, 1, 0, 0, 0, 1, 0, 1, 1, 1, 0, 0, 0, 1, 0, 1, 1, 1, 0, 0, 1, 0, 1, 1, 0, 0, 1, 1, 0, 0, 1, 1, 0, 0, 1, 1, 1, 0, 1, 1, 1, 0, 0, 1, 0, 1, 1, 1, 0, 0, 1, 0, 1, 1, 1, 0, 0, 1, 0, 1, 1, 1, 0, 1, 0, 0, 0, 0, 0 ] | ||
| } |
There was a problem hiding this comment.
Rather than replicating a lot of the logic of the implementation in the test, we could use hardcoded arrays for the expected values.
hectorBrown
force-pushed
the
platonic_udd
branch
from
809bcb6 to
0effe70
Compare
|
Hi, appreciate you taking the time to read over this. I've made the changes you suggested above. I also moved from generating the sequences with python generators to numpy for speed and readability. I worry that the tests are a bit unwieldy, especially the Icosahedral one - would it be possible to store these arrays elsewhere? |
3 participants
Changes proposed in this pull request: