pasqal-io / Pulser

Library for pulse-level/analog control of neutral atom devices. Emulator with QuTiP.
Apache License 2.0
159 stars 57 forks source link

Delete summing constraint in DetuningMap #610

Closed a-corni closed 7 months ago

a-corni commented 7 months ago

Deleting the condition "sum of weights = 1" in DetuningMap. Condition is replaced by "weights are between 0 and 1". Impacts the creation of the DetuningMap associated with an SLM (used to be with weights = 1/(#number of masked qubits), is now created with weights = 1 for masked qubits, 0 otherwise). Notebooks are updated. Is proposed to be a hotfix to advertise about this change among the developers.

Possible Improvement to this PR: To really make people aware that the summing constraint has been lifted, we could show a DeprecationWarning if the sum of the weights is equal to 1 ?

Fixes #609

review-notebook-app[bot] commented 7 months ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

madagra commented 7 months ago

@a-corni thank you for this PR. Can I ask why exactly this constraint has been lifted? Is it not valid anymore?

a-corni commented 7 months ago

@a-corni thank you for this PR. Can I ask why exactly this constraint has been lifted? Is it not valid anymore?

It has in fact never been valid, @Lassabliere just realized that there had been a misunderstanding while discussing with a user who was telling him about this constraint.

a-corni commented 7 months ago

@a-corni I am not in favor of fully removing the constraint on the weights. The alternative solution discussed offline might be more appropriate. For what concerns the actual changes in this PR, they look correct to me.

@madagra everything should be now tackled. @Lassabliere, bottom_detuning in notebook and devices is updated to -20 2 pi rad/µs and global bottom detuning is -2000 2 pi rad/µs.

In the end we don't have to delete the condition of the sum of the weights being equal to 1, we only need a constraint on the local detuning and a constraint on the global detuning. However, I think this implementation will help the user since it will be closer to how Local channels used to work (with weights equal to 1 or 0, you can define directly the detuning you want to apply on each qubit), even though this is making us drive away from the initial concept of a Global channel (where the detuning defined by the user should be between global_bottom_detuning and 0).

Lassabliere commented 7 months ago

It won't be very user-friendly to work with weights.

For example, if you're working with 30 atoms and you have a DMM with a total available light shift of -2pi x 2000 MHz. In general, we want to work with detunings of the order of - 2pi x 1 MHz on the atoms, so you need to set very small weights [0.0005, ....]. Moreover algorithm developers will think in terms of local light-shift and not in term of weights (except for ramps maybe). Or, at least you can create a function that returns weights for a given light-shift.

Why don't you want to work with an additional term $-\hbar^2/2 \sum_i \Delta_i \sigma^z_i$ instead of what we have currently with a condition $\sum_i \Deltai \le \Delta{tot}$ ?

a-corni commented 7 months ago

Hey @Lassabliere,

There might have been a misunderstanding with my previous message. I definitely agree with your point, and your example highlights it very well 😄

Indeed the former implementation (defining the total lightshift applied on the qubits, applying on it weights whose sum is equal to 1 to obtain the detuning applied on each qubit) has the issue that the weights will be small if the number of atoms that have to receive light is large (to apply a detuning of -2pi rad/µs on each atom, you need to define a waveform with detuning -2pi30 rad/µs and a DetuningMap with weights $1/30 \approx 0.033$ on each atom).

Now the implementation under scrutiny here defines weights indeed, but that will generally be close to 1. To perform the operation you describe, we now send a waveform of detuning -2pi rad/µs on a DetuningMap with weights 1 on each atom, and we now internally check that ${\sum_{i=1}}^{30}(-2\pi) = -60\pi < -2 \pi 2000$. I find this very user-friendly. Another advantage of this solution is that whatever user did before with sum = 1 will still work.

Lassabliere commented 7 months ago

My bad, I didn't read everything carefully. It looks fine for me!

a-corni commented 7 months ago

@HGSilveri my modifications are ready for review :)

HGSilveri commented 7 months ago

Possible Improvement to this PR: To really make people aware that the summing constraint has been lifted, we could show a DeprecationWarning if the sum of the weights is equal to 1 ?

I forgot to comment on this: I think it's too noisy, I'd be more in favor of adding a note in a docstring signaling this change.

a-corni commented 7 months ago

Possible Improvement to this PR: To really make people aware that the summing constraint has been lifted, we could show a DeprecationWarning if the sum of the weights is equal to 1 ?

I forgot to comment on this: I think it's too noisy, I'd be more in favor of adding a note in a docstring signaling this change.

Yes in the end I think it's irrelevant, let's just keep it this way isn't it ?