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

Give access to all EOM block parameters and allow for phase drift correction #566

Merged
merged 8 commits into from
Aug 29, 2023

Conversation

HGSilveri
Copy link
Collaborator

@HGSilveri HGSilveri commented Aug 1, 2023

  • Give access to the detuning_off value through RydbergEOM.calculate_detuning_off()
  • Add a correct_phase_drift: bool option to EOM operations to correct for the phase drift that occurs during detuned delays
  • Update the abstract representation JSON schema to support the new signature of the EOM operations

Fixes #562 .

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.

@HGSilveri thank you so much for working on this so quickly.

I just had a quick look without working out all the logic here since there is a lot of new code.

If I understand correctly, there is now a new option, off by default, to automatically correct the phase drift acquired by delays before new pulses when in EOM mode, so that the user get an effective pulse that has the intended effect.

The workaround we have with Lucas at the moment is to find the off detuning value of the sequence in EOM mode by some means, add a delay of known duration between pulses, and do the phase correction on the user side.

We have to make the delay long enough so that the phase can ramp up.
Is the ramp up for the correction of the phase taken into account when scheduling the pulses?

pulser-core/pulser/sequence/_schedule.py Show resolved Hide resolved
pulser-core/pulser/sequence/sequence.py Show resolved Hide resolved
pulser-core/pulser/sequence/sequence.py Show resolved Hide resolved
pulser-core/pulser/sequence/sequence.py Show resolved Hide resolved
pulser-core/pulser/sequence/sequence.py Show resolved Hide resolved
@HGSilveri
Copy link
Collaborator Author

@HGSilveri thank you so much for working on this so quickly.

I just had a quick look without working out all the logic here since there is a lot of new code.

Hey @lvignoli , thanks for the quick review! I just wanted to let you know about this feature to make sure it suits your needs, don't worry about reviewing everything in detail.

If I understand correctly, there is now a new option, off by default, to automatically correct the phase drift acquired by delays before new pulses when in EOM mode, so that the user get an effective pulse that has the intended effect.

That is correct, yes.

The workaround we have with Lucas at the moment is to find the off detuning value of the sequence in EOM mode by some means, add a delay of known duration between pulses, and do the phase correction on the user side.

We have to make the delay long enough so that the phase can ramp up. Is the ramp up for the correction of the phase taken into account when scheduling the pulses?

Yes, whenever you modify the phase the so called "phase jump time" is enforced and this is no exception. In fact, if the delay causing the phase drift you're trying to correct for is too short to change the phase, it is extend by the necessary amount and then the phase drift correction takes into account the full delay, not just the original one.

@HGSilveri HGSilveri marked this pull request as ready for review August 2, 2023 10:51
@lvignoli
Copy link
Collaborator

lvignoli commented Aug 3, 2023

Okay it makes things clear on my side. Also I am glad to know the phase ramp up I always taken into account (Pulser is doing so much behind the scenes during pulse scheduling, it's sometimes hard to know what is guaranteed and what is not).

pulser-core/pulser/channels/eom.py Show resolved Hide resolved
pulser-core/pulser/sequence/sequence.py Show resolved Hide resolved
pulser-core/pulser/sequence/sequence.py Show resolved Hide resolved
tests/test_sequence.py Show resolved Hide resolved
@bejito
Copy link
Member

bejito commented Aug 22, 2023

correct_phase_drift seems to be an operation applied in the frontend during the design of the Pulses. Is there a reason for it to be serialized ? Instead of having the final corrected pulse values serialized I mean.

@HGSilveri
Copy link
Collaborator Author

correct_phase_drift seems to be an operation applied in the frontend during the design of the Pulses. Is there a reason for it to be serialized ? Instead of having the final corrected pulse values serialized I mean.

It's because of the parametrized sequences, since you can't know before the sequence is built what the correction factor will be.

@bejito
Copy link
Member

bejito commented Aug 22, 2023

Ah this makes sense indeed. So for non-parametrized sequences, do we directly apply the correction and serialize it or do we let the backend apply the correction ?

@HGSilveri
Copy link
Collaborator Author

Ah this makes sense indeed. So for non-parametrized sequences, do we directly apply the correction and serialize it or do we let the backend apply the correction ?

We let the backend apply the correction

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@a-corni
Copy link
Collaborator

a-corni commented Aug 28, 2023

I am ready to approve, everything looks good to me. There seems to be an issue with the build of the docs, I wonder if it was not just a bug with the imports.

@HGSilveri
Copy link
Collaborator Author

I am ready to approve, everything looks good to me. There seems to be an issue with the build of the docs, I wonder if it was not just a bug with the imports.

Yes, this seems to be a general issue, I don't think it's related to these changes. I'll look into it

@HGSilveri HGSilveri merged commit 2315989 into develop Aug 29, 2023
6 checks passed
@HGSilveri HGSilveri deleted the hs/eom-params branch August 29, 2023 13:07
@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.

Provide public access to the settings of each EOM block
4 participants