Delete summing constraint in DetuningMap #610

a-corni · 2023-11-10T16:30:55Z

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 · 2023-11-10T16:30:59Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

madagra · 2023-11-13T09:14:46Z

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

a-corni · 2023-11-13T09:20:33Z

@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.

madagra

@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.

a-corni · 2023-11-14T16:21:55Z

@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 · 2023-11-15T12:28:39Z

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 \Delta_i \le \Delta_{tot}$ ?

a-corni · 2023-11-15T13:29:44Z

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 · 2023-11-15T13:47:24Z

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

pulser-core/pulser/channels/dmm.py

pulser-core/pulser/json/abstract_repr/schemas/device-schema.json

…lete_constraint_Detmap

HGSilveri

I still need to review part of the changes but I wanted to get these comments out of the way first so we can discuss them

pulser-core/pulser/channels/dmm.py

madagra

@a-corni it looks good to me now, I will leave @HGSilveri to give the final approval.

HGSilveri

As far as I can tell we still need to be more thorough with the abstract representation tests (especially if we are to try and maintain backwards compatibility)

pulser-core/pulser/sequence/sequence.py

HGSilveri · 2023-11-24T08:34:01Z

pulser-core/pulser/sequence/sequence.py

+                    call.name == "config_detuning_map"
+                    or call.name == "config_slm_mask"
+                ):
+                    # Check whether dmm_name matches with channel


It seems that nothing happens if there is no match though, is that intended?

Is it really okay that we are failing silently?

It's would say it's not supposed to fail: the channel has been tested by validate_channel before being as being in declared_channels, and declared_channels is exactly built by looping over the calls.

Ok, good enough for me

Actually, let's add an assert in that case to ensure the detuning map is always found

pulser-core/pulser/sequence/sequence.py

tutorials/advanced_features/Local addressability with DMM.ipynb

…lete_constraint_Detmap

a-corni · 2023-11-27T16:06:00Z

@HGSilveri my modifications are ready for review :)

HGSilveri

Final remarks :)

pulser-core/pulser/devices/_devices.py

tests/test_abstract_repr.py

HGSilveri · 2023-11-27T17:35:01Z

pulser-core/pulser/sequence/sequence.py

+                    call.name == "config_detuning_map"
+                    or call.name == "config_slm_mask"
+                ):
+                    # Check whether dmm_name matches with channel


Is it really okay that we are failing silently?

HGSilveri · 2023-11-28T08:57:00Z

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 · 2023-11-28T09:57:31Z

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 ?

HGSilveri

Great, LGTM!

Main changes: a0c59a6 Deprecate legacy devices (#620) b18e434 FIX: Detuning modulation for custom EOM buffer time (#619) 8c5d966 Delete summing constraint in DetuningMap (#610) b41d62a JSON serialization support for numpy integer types (#617) 9f9a17a Fix temperature initialization in SimConfig (#606) 2bad427 Fix spacing in register.max_connectivity (#605) 581886f [FIX] Allow parametrized phase in EOM mode (#599) 394fe6a Add `wait` option to remote backends (#598)

Delete summing constraint DetuningMap

5cfc5aa

a-corni requested review from madagra and Lassabliere November 10, 2023 16:30

Bump version to 0.15.3

98e9ba4

a-corni changed the base branch from master to develop November 10, 2023 16:46

Reverting version to v0.16.2

24c0b91

madagra reviewed Nov 13, 2023

View reviewed changes

a-corni added 5 commits November 14, 2023 11:39

Add global_bottom_detuning

674923d

Update schema

f157566

Updating documentation

f8fcc79

Fix typing, tests

7909679

Add global_bottom_det on slm, switch device

d0906b0

madagra mentioned this pull request Nov 15, 2023

[Feature] Add semi-local addressing pasqal-io/qadence#184

Merged

madagra reviewed Nov 15, 2023

View reviewed changes

a-corni added 3 commits November 16, 2023 17:09

Update descr bottom_det, dmm,

67d14b0

Fixing test of array

8a64c78

replace array comparison by min comparisons

32aa5ed

a-corni self-assigned this Nov 20, 2023

a-corni requested a review from HGSilveri November 20, 2023 14:12

a-corni added 2 commits November 20, 2023 17:55

Merge branch 'develop' of https://github.com/pasqal-io/Pulser into de…

cb3c12d

…lete_constraint_Detmap

global_bottom_detuning to total_bottom_detuning

852aa6e

HGSilveri reviewed Nov 23, 2023

View reviewed changes

pulser-core/pulser/channels/dmm.py Outdated Show resolved Hide resolved

pulser-core/pulser/channels/dmm.py Outdated Show resolved Hide resolved

pulser-core/pulser/channels/dmm.py Outdated Show resolved Hide resolved

pulser-core/pulser/channels/dmm.py Outdated Show resolved Hide resolved

madagra reviewed Nov 24, 2023

View reviewed changes

HGSilveri reviewed Nov 24, 2023

View reviewed changes

tutorials/advanced_features/Local addressability with DMM.ipynb Show resolved Hide resolved

tutorials/advanced_features/Local addressability with DMM.ipynb Show resolved Hide resolved

a-corni added 3 commits November 24, 2023 12:10

Taking into account review comments

67d9242

Modify schema, add deprecate warning

d310956

Fix imports

1fe5581

HGSilveri added this to the v0.16 Release milestone Nov 27, 2023

a-corni added 3 commits November 27, 2023 16:45

Make total_bottom_detuning optional in abstr_repr

1e84c39

Test de- & serializ. of dev with total_bottom_det

5749976

Merge branch 'develop' of https://github.com/pasqal-io/Pulser into de…

4baf3cd

…lete_constraint_Detmap

HGSilveri reviewed Nov 27, 2023

View reviewed changes

a-corni added 2 commits November 28, 2023 10:57

Modify warning handling

cc306ae

Adding assertion in _validate_and_adjust_pulse

cc3c857

HGSilveri approved these changes Nov 28, 2023

View reviewed changes

a-corni merged commit 8c5d966 into develop Nov 28, 2023
7 checks passed

HGSilveri mentioned this pull request Dec 1, 2023

Release v0.16.0 #621

Merged

HGSilveri deleted the delete_constraint_Detmap branch December 4, 2023 12:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delete summing constraint in DetuningMap #610

Delete summing constraint in DetuningMap #610

a-corni commented Nov 10, 2023

review-notebook-app bot commented Nov 10, 2023

madagra commented Nov 13, 2023

a-corni commented Nov 13, 2023

madagra left a comment

a-corni commented Nov 14, 2023 •

edited

Loading

Lassabliere commented Nov 15, 2023 •

edited

Loading

a-corni commented Nov 15, 2023 •

edited

Loading

Lassabliere commented Nov 15, 2023 •

edited

Loading

HGSilveri left a comment

madagra left a comment

HGSilveri left a comment

HGSilveri Nov 24, 2023

HGSilveri Nov 27, 2023

a-corni Nov 28, 2023

HGSilveri Nov 28, 2023

HGSilveri Nov 28, 2023

a-corni commented Nov 27, 2023

HGSilveri left a comment

HGSilveri Nov 27, 2023

HGSilveri commented Nov 28, 2023

a-corni commented Nov 28, 2023

HGSilveri left a comment

Delete summing constraint in DetuningMap #610

Delete summing constraint in DetuningMap #610

Conversation

a-corni commented Nov 10, 2023

review-notebook-app bot commented Nov 10, 2023

madagra commented Nov 13, 2023

a-corni commented Nov 13, 2023

madagra left a comment

Choose a reason for hiding this comment

a-corni commented Nov 14, 2023 • edited Loading

Lassabliere commented Nov 15, 2023 • edited Loading

a-corni commented Nov 15, 2023 • edited Loading

Lassabliere commented Nov 15, 2023 • edited Loading

HGSilveri left a comment

Choose a reason for hiding this comment

madagra left a comment

Choose a reason for hiding this comment

HGSilveri left a comment

Choose a reason for hiding this comment

HGSilveri Nov 24, 2023

Choose a reason for hiding this comment

HGSilveri Nov 27, 2023

Choose a reason for hiding this comment

a-corni Nov 28, 2023

Choose a reason for hiding this comment

HGSilveri Nov 28, 2023

Choose a reason for hiding this comment

HGSilveri Nov 28, 2023

Choose a reason for hiding this comment

a-corni commented Nov 27, 2023

HGSilveri left a comment

Choose a reason for hiding this comment

HGSilveri Nov 27, 2023

Choose a reason for hiding this comment

HGSilveri commented Nov 28, 2023

a-corni commented Nov 28, 2023

HGSilveri left a comment

Choose a reason for hiding this comment

a-corni commented Nov 14, 2023 •

edited

Loading

Lassabliere commented Nov 15, 2023 •

edited

Loading

a-corni commented Nov 15, 2023 •

edited

Loading

Lassabliere commented Nov 15, 2023 •

edited

Loading