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

Clarify the Conventions in Pulser #573

Merged
merged 19 commits into from
Sep 15, 2023
Merged

Clarify the Conventions in Pulser #573

merged 19 commits into from
Sep 15, 2023

Conversation

HGSilveri
Copy link
Collaborator

  • Adds a "Conventions" page to the docs, establishing all the conventions in Pulser
  • Modifies the simulation in XY mode to comply with this convention

Closes #571 .

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link
Collaborator

@a-corni a-corni left a comment

Choose a reason for hiding this comment

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

Great documentation, as far as I am concerned it is very clear and it will for sure help users :) Just found a minor typo and have two suggestions

pulser-simulation/pulser_simulation/qutip_result.py Outdated Show resolved Hide resolved
docs/source/conventions.rst Outdated Show resolved Hide resolved
@madagra
Copy link
Collaborator

madagra commented Sep 5, 2023

@HGSilveri I have only a couple of small comments but it all looks good to me:

  • I would add a note about the endianness to which the chosen basis state ordering corresponds when expressing bitstrings. I think it is little endian, if I am not mistaken. The "Measurements samples ordering" is probably the right place for it.
  • In the XY basis, however, the state ordering looks flipped to me, now you get big endian when you sample. I guess the explanation given to @a-corni is valid also in this case but I want to point out this since it can be pretty dangerous in some cases.

@HGSilveri
Copy link
Collaborator Author

@HGSilveri I have only a couple of small comments but it all looks good to me:

  • I would add a note about the endianness to which the chosen basis state ordering corresponds when expressing bitstrings. I think it is little endian, if I am not mistaken. The "Measurements samples ordering" is probably the right place for it.
  • In the XY basis, however, the state ordering looks flipped to me, now you get big endian when you sample. I guess the explanation given to @a-corni is valid also in this case but I want to point out this since it can be pretty dangerous in some cases.

Thanks for the review @madagra ! I'm not sure I understand your comments. From my understanding, endianness refers to how a binary number is stored in a register and displayed, so it is only relevant when you are reading your samples as a binary number. I think it's preferable to leave this up to the user and stick solely to matching the samples to the order of the qubit IDs. Let me give you an example:

Take a register with qubit IDs ("fish", "cat", "dog"). The state vector is going to be |fish, cat, dog> and let's say that upon measurement in the ground-rydberg basis it is projected onto |rgg>. Then, we will register a count in sample 100. That's all. The user can then decide whether "dog" or "fish" is the smallest-memory site in their Register and this will determine whether they read the sample as "4" or "1".

Also, can you point me to where ordering is flipped in the XY basis? This shouldn't be the case.

@lvignoli
Copy link
Collaborator

lvignoli commented Sep 6, 2023

I had a quick read without checking the validity of the maths.
As a user that comes back to these computations regularly, I would say most of the useful information is there. Nice job!

Copy link
Collaborator

@lvignoli lvignoli left a comment

Choose a reason for hiding this comment

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

I in fact had this small comment

docs/source/conventions.rst Show resolved Hide resolved
@a-corni
Copy link
Collaborator

a-corni commented Sep 14, 2023

Nit: There is a reference to conventions page in the notebook about SLM

according to Pulser's conventions for XY basis states

Perhaps we could link both pages ?

@HGSilveri HGSilveri added this to the v0.15 Release milestone Sep 14, 2023
@a-corni
Copy link
Collaborator

a-corni commented Sep 15, 2023

This looks good to me !

@HGSilveri
Copy link
Collaborator Author

This looks good to me !

I was still waiting for a reply from @madagra, but I guess I can merge and then make modifications later if necessary

@HGSilveri HGSilveri merged commit 5270944 into develop Sep 15, 2023
6 checks passed
@HGSilveri HGSilveri deleted the hs/conventions branch September 15, 2023 15:35
@HGSilveri HGSilveri mentioned this pull request Sep 27, 2023
HGSilveri added a commit that referenced this pull request Sep 27, 2023
Main changes:

e0943d9 Override `optimal_detuning_off` on stored calls (#588)
3e40319 Deprecate legacy serializer + Improve error messages (#585)
9e05982 Adding register_is_from_calibrated_layout and is_calibrated_layout to Device (#586)
c08dfa8 Adding dmm config and modulation to sequence (#564)
5270944 Clarify the Conventions in Pulser (#573)
2315989 Give access to all EOM block parameters and allow for phase drift correction (#566)
d5ac020 Adding  DetuningMap, DMM (#539)
f56a19f Remove expired deprecations in pulser-pasqal
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.

Write a "Conventions" page
4 participants