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

New sampler implementation #388

Merged
merged 34 commits into from
Jul 29, 2022
Merged

Conversation

lvignoli
Copy link
Collaborator

@lvignoli lvignoli commented Jul 19, 2022

The following sampler implementation is channel-centric—by opposing to the former one which was qubit centric.

Samples dataclasses are defined in pulser-core/samples.py. Samples are extracted at the channel level from the _ChannelSchedule.get_samples() method. For the regular user, there is only one function to know, the sample(seq) one exposed by the pulser-core/sampler/ module.

Improvements:

  • tests
  • creation of ChannelDrawContent from ChannelSamples.
    There is much more info needed to draw, its a wider scope than the time series, so I am unsure about the right design.
  • method on the Sequence class to get samples?

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@lvignoli lvignoli requested a review from HGSilveri July 19, 2022 13:54
@lvignoli
Copy link
Collaborator Author

Last commits restore the tests and update them to the new architecture.
They actually allow to catch an issue with channel duration not being the same as the parent
sequence hence messing up the samples arrays.

@HGSilveri
Copy link
Collaborator

Thank you @lvignoli ! I'll take over from here and circle back to you if I have any questions

@HGSilveri HGSilveri self-assigned this Jul 20, 2022
@lvignoli lvignoli mentioned this pull request Jul 20, 2022
4 tasks
@HGSilveri HGSilveri marked this pull request as ready for review July 26, 2022 08:20
@HGSilveri
Copy link
Collaborator

@lvignoli the changes are done. I'm noticing there's a tutorial that I'm guessing should be untracked?

@lvignoli
Copy link
Collaborator Author

lvignoli commented Jul 26, 2022

Thank you @HGSilveri!
I'll review them soon.

The tutorial was simply to check that the sampler was working as intended, so you can delete it safely. Besides I think @arthurfaria would like to write a notebook tutorial once this is reviewed and merge, see #349.

@arthurfaria
Copy link

Thank you @HGSilveri! I'll review them soon.

The tutorial was simply to check that the sampler was working as intended, so you can delete it safely. Besides I think @arthurfaria would like to write a notebook tutorial once this is reviewed and merge, see #349.

Sounds good. Perhaps I will start with that one. Is there any other channel we can communicate like slack or something?

@HGSilveri
Copy link
Collaborator

Sounds good. Perhaps I will start with that one. Is there any other channel we can communicate like slack or something?

Indeed, we have a Slack channel that has been dormant but that we can use. I sent you an invite.

Copy link
Collaborator Author

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

Up to my few comments, everything looks good to me!
Thank you for taking over the development 💪

NB: I cannot approve since I am the author of the PR

pulser-core/pulser/sampler/sampler.py Outdated Show resolved Hide resolved
pulser-core/pulser/sampler/sampler.py Show resolved Hide resolved
pulser-core/pulser/sampler/samples.py Show resolved Hide resolved
pulser-core/pulser/sampler/samples.py Show resolved Hide resolved
pulser-core/pulser/sampler/samples.py Show resolved Hide resolved
pulser-core/pulser/sequence/_seq_drawer.py Show resolved Hide resolved
pulser-core/pulser/sequence/_seq_drawer.py Show resolved Hide resolved
pulser-core/pulser/sequence/_seq_drawer.py Outdated Show resolved Hide resolved
tests/test_sequence_sampler.py Outdated Show resolved Hide resolved
tests/test_sequence_sampler.py Show resolved Hide resolved
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.

Approved with @lvignoli 's blessing 🙏

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.

3 participants