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.

Sign up for GitHub

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

Jump to bottom

Rectangular lattice register and layout #665

Merged
merged 7 commits into from
Apr 29, 2024
Merged

Rectangular lattice register and layout #665

merged 7 commits into from
Apr 29, 2024

Conversation

soufianekaghad98
Copy link
Contributor

Addition of classes to the special layouts :
RectangularLatticeLayout to be able to play with x and y spacing. Now SquareLatticeLayout inherits from it. Don't know how to deal with the print change, we can keep rectangular for a square. (one test fails that compares the strings)
TriangularLatticeLayoutRectShape to have triangular lattice in a rectangular layout
RandomLayout that generates a random layout for a given number of traps (limited number of iterations)

mypy failed because of no type defintion of an empty list in RandomLayout.

@HGSilveri
Copy link
Collaborator

Hi @soufianekaghad98 , thank you for your contribution! I have a few preliminary comments and questions:

  1. I agree with the addition of RectangularLatticeLayout but I'm not sure about the others.
  2. In your opinion, what is the advantage of having a triangular lattice in a rectangular shape? Note that you can still define rectangular-shaped registers in the standard triangular lattice
  3. RandomLayout is problematic because layouts rely on reproducibility, meaning that the same call should yield identical results. This will naturally not be the case for a random layout. Would it be so bad for the user to randomly generate the set of coordinates given to RegisterLayout instead?

HGSilveri
HGSilveri reviewed Mar 29, 2024
View reviewed changes
Copy link
Collaborator

@HGSilveri HGSilveri left a comment

Choose a reason for hiding this comment

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

Here goes a first batch of comments

pulser-core/pulser/register/special_layouts.py Show resolved Hide resolved
pulser-core/pulser/register/special_layouts.py Outdated Show resolved Hide resolved
pulser-core/pulser/register/special_layouts.py Outdated Show resolved Hide resolved
tests/test_register_layout.py Outdated Show resolved Hide resolved
@soufianekaghad98
Copy link
Contributor Author

Hi @HGSilveri, thanks for your feedback!
To give you some context, I am currently working on the equalization of the traps. So my goal is to generate a whole bunch of different layouts and test them to see how well we can get equalized traps.
This is why I want to use special_layouts which gives different kinds of patterns ready to use, and triangular lattice in rectangular shape is one among the easy layouts to try. I agree that in the end for a whole sequence it will suffice to just have the rectangular register, but for now I'm only working with layouts).
I feel like RandomLayout could be useful for other people who like me need to generate a lot of random layouts with given constraints (mostly for benchmarking and testing). Maybe it could be placed somewhere else then?

@HGSilveri
Copy link
Collaborator

This is why I want to use special_layouts which gives different kinds of patterns ready to use, and triangular lattice in rectangular shape is one among the easy layouts to try. I agree that in the end for a whole sequence it will suffice to just have the rectangular register, but for now I'm only working with layouts).
I feel like RandomLayout could be useful for other people who like me need to generate a lot of random layouts with given constraints (mostly for benchmarking and testing). Maybe it could be placed somewhere else then?

Yes, I think RandomLayout should be placed at a higher-level package (which could be wherever you are using Pulser). TriangularLatticeRectShape could be included here but since you are already defining RandomLayout somewhere else, maybe it could go there as well?
I'm not strongly opposed to the inclusion of TriangularLatticeRectShape, I just think its utility will be very limited and it can easily end up being something we support but nobody uses after a while.

HGSilveri
HGSilveri reviewed Apr 3, 2024
View reviewed changes
pulser-core/pulser/json/supported.py Show resolved Hide resolved
pulser-core/pulser/register/__init__.py Outdated Show resolved Hide resolved
pulser-core/pulser/register/register.py Outdated Show resolved Hide resolved
pulser-core/pulser/register/register.py Outdated Show resolved Hide resolved
pulser-core/pulser/register/register.py Outdated Show resolved Hide resolved
pulser-core/pulser/register/register.py Outdated Show resolved Hide resolved
pulser-core/pulser/register/register.py Outdated Show resolved Hide resolved
pulser-core/pulser/register/special_layouts.py Outdated Show resolved Hide resolved
@review-notebook-app Review Notebook App
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@soufianekaghad98
Copy link
Contributor Author

Feels like I messed up when trying to push again (apparently there was some issue with my local branch not being up to date with the remote... I tried different commands to pull then push which ended with me pushing stuff I didn't know about...). Sorry for the mess.

@HGSilveri
Copy link
Collaborator

Feels like I messed up when trying to push again (apparently there was some issue with my local branch not being up to date with the remote... I tried different commands to pull then push which ended with me pushing stuff I didn't know about...). Sorry for the mess.

No worries, it's nothing we can't fix. I'll give you a hand when I can

@soufianekaghad98 soufianekaghad98 force-pushed the rectangular_layout branch 2 times, most recently from 8ef21d6 to 4d54ee4 Compare April 26, 2024 16:03
HGSilveri
HGSilveri reviewed Apr 26, 2024
View reviewed changes
Copy link
Collaborator

@HGSilveri HGSilveri left a comment

Choose a reason for hiding this comment

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

Almost there, these should be the final comments before I approve :)

pulser-core/pulser/json/supported.py Outdated Show resolved Hide resolved
pulser-core/pulser/register/register.py Show resolved Hide resolved
@HGSilveri HGSilveri changed the title Rectangular lattice and random layouts Rectangular lattice register and layout Apr 29, 2024
HGSilveri
HGSilveri approved these changes Apr 29, 2024
View reviewed changes
Copy link
Collaborator

@HGSilveri HGSilveri left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot @soufianekaghad98 !

@HGSilveri HGSilveri merged commit c695373 into pasqal-io:develop Apr 29, 2024
7 checks passed
@HGSilveri HGSilveri mentioned this pull request Apr 29, 2024
Merged
HGSilveri added a commit that referenced this pull request Apr 29, 2024
@HGSilveri
Release v0.18.0
82c282b 
Main changes:

21a47f3 Remove Register.rotate() (#642)
20e6765 FIX: Redefine slope of RampWaveform (#644)
c2d5b6c Enabling definition of multiple noise channels and noise channels in XY (#647)
bcb78cc Enable digital simulation (#652)
0f6e3dd Improve access to output modulation durations (#663)
188d21d Remove deprecated noise arguments (#674)
f303138 Adding relaxation noise channel (#675)
716b86b Centralize all backend imports from a single pulser.backends module (#678)
96a8c34 Add hyperfine dephasing rate to NoiseModel (#680)
4981ca6 Add optional default noise models to devices (#676)
c695373 Rectangular lattice register and layout (#665)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HGSilveri HGSilveri HGSilveri approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@soufianekaghad98 @HGSilveri