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

Incorporate automatic layout generation in Register #753

Merged
merged 9 commits into from
Oct 17, 2024

Conversation

HGSilveri
Copy link
Collaborator

  • Adds an algorithm for defining the layout trap coordinates from a set of atomic coordinates
  • Defines Register.with_automatic_layout(device: Device), which allows the user to get an automatically generated layout that obeys the device constrains from any register

Closes #748 .

@HGSilveri HGSilveri requested a review from a-corni October 16, 2024 12:53
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@HGSilveri HGSilveri changed the title Incoporate automatic layout generation in Register Incorporate automatic layout generation in Register Oct 16, 2024
pulser-core/pulser/register/_layout_gen.py Outdated Show resolved Hide resolved
pulser-core/pulser/register/_layout_gen.py Outdated Show resolved Hide resolved
pulser-core/pulser/register/_layout_gen.py Show resolved Hide resolved
pulser-core/pulser/register/_layout_gen.py Outdated Show resolved Hide resolved
min_traps,
)
if max_traps:
target_traps = min(target_traps, max_traps)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't it also possible that min_traps be bigger than max_traps ? Shouldn't we also have
min_traps = min(min_traps, max_traps)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When they come from Device it's not possible; min_traps > max_traps has no solution, after all

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This function is not supposed to be user-facing so I'll just add some asserts for now

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah I think there was a little misunderstanding. I meant that if N_seeds is extremely big, you might have the new min_traps higher than max_traps after the np.ceil, so I guess it would be good to have

Suggested change
target_traps = min(target_traps, max_traps)
target_traps = min(target_traps, max_traps)
min_traps = min(min_traps, max_traps)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see what you mean but if you do this, the program will successfully terminate when it reach max_traps, which will be lower than what you need (min_traps before your new line). We don't want this to happen because it won't respect the max_layout_filling.
What I'll do instead is always check at the end that len(traps) >= min_traps and raise the RuntimeError otherwise.

pulser-core/pulser/register/_layout_gen.py Outdated Show resolved Hide resolved
@HGSilveri HGSilveri merged commit 2fa0f7f into develop Oct 17, 2024
9 checks passed
@HGSilveri HGSilveri deleted the hs/arb-layout-gen branch October 17, 2024 16:10
@HGSilveri HGSilveri mentioned this pull request Oct 17, 2024
HGSilveri added a commit that referenced this pull request Oct 18, 2024
**Main changes**:
- Add layout restricting parameters to BaseDevice (#751) 
- Incorporate automatic layout generation in Register (#753) 

**Fixes**:
- Fix rounding error in RampWaveform (#747) 
- Fix normalisation in ring of atoms (#750)
- Improve the NoiseModel unused parameters warning message (#752)
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.

Add a way to generate a layout from a register
2 participants