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

AbstractSampler Interface #12

Merged
merged 32 commits into from
Nov 12, 2023
Merged

AbstractSampler Interface #12

merged 32 commits into from
Nov 12, 2023

Conversation

tjungni
Copy link
Member

@tjungni tjungni commented Oct 6, 2023 •
edited
Loading

TODO:

  • Update QEDprocesses in QEDevents on released version
  • Update QEDprocesses in QEDevents/test on released version
  • Remove qedprocesses_setup_interface.jl moc file
  • Replace QEDevents. with QEDprocesses. and remove at QEDevents.compute

@szabo137 szabo137 mentioned this pull request Oct 6, 2023
Closed
@tjungni tjungni mentioned this pull request Oct 13, 2023
Merged
szabo137
szabo137 reviewed Oct 16, 2023
View reviewed changes
Copy link
Member

@szabo137 szabo137 left a comment

Choose a reason for hiding this comment

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

First comments and questions.

src/interfaces/sampler_interface.jl Outdated Show resolved Hide resolved
src/interfaces/sampler_interface.jl Outdated Show resolved Hide resolved
src/interfaces/sampler_interface.jl Outdated Show resolved Hide resolved
src/interfaces/sampler_interface.jl Outdated Show resolved Hide resolved
src/interfaces/sampler_interface.jl Outdated Show resolved Hide resolved
test/interfaces.jl Outdated Show resolved Hide resolved
test/interfaces.jl Outdated Show resolved Hide resolved
test/interfaces.jl Outdated Show resolved Hide resolved
test/interfaces.jl Outdated Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
szabo137
szabo137 reviewed Oct 18, 2023
View reviewed changes
Copy link
Member

@szabo137 szabo137 left a comment

Choose a reason for hiding this comment

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

More comments. Furthermore, consider renaming test/interfaces.jl to test/interfaces/sampler_interface.jl. There will be more interfaces to be tested.

src/interfaces/sampler_interface.jl Outdated Show resolved Hide resolved
src/interfaces/sampler_interface.jl Outdated Show resolved Hide resolved
src/interfaces/sampler_interface.jl Outdated Show resolved Hide resolved
test/interfaces.jl Outdated Show resolved Hide resolved
test/interfaces.jl Outdated Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
szabo137
szabo137 requested changes Oct 28, 2023
View reviewed changes
Copy link
Member

@szabo137 szabo137 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

In general, the PR looks good to me, and after resolving the _rand! piracy, it can be merged.

@tjungni could you please update the bullet points in the description?

src/interfaces/sampler_interface.jl Outdated Show resolved Hide resolved
@szabo137 szabo137 marked this pull request as ready for review November 2, 2023 21:06
szabo137
szabo137 reviewed Nov 7, 2023
View reviewed changes
Copy link
Member

@szabo137 szabo137 left a comment

Choose a reason for hiding this comment

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

One last comment, then I am happy to merge this!

src/interfaces/sampler_interface.jl Show resolved Hide resolved
szabo137
szabo137 previously approved these changes Nov 7, 2023
View reviewed changes
Copy link
Member

@szabo137 szabo137 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for providing this PR, it looks fine to me now and is ready for merging.

@tjungni Could you please run the formatter on the last version of your contribution?

AntonReinhard
AntonReinhard requested changes Nov 8, 2023
View reviewed changes
Copy link
Member

@AntonReinhard AntonReinhard left a comment

Choose a reason for hiding this comment

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

Is there any specific reason why the parameters are called smplr instead of sampler everywhere? Am I not seeing a name shadowing? Otherwise, it seems a little obtuse for 2 saved characters.

src/interfaces/sampler_interface.jl Show resolved Hide resolved
src/interfaces/sampler_interface.jl Outdated Show resolved Hide resolved
src/interfaces/sampler_interface.jl Outdated Show resolved Hide resolved
test/interfaces/sampler_interface.jl Show resolved Hide resolved
@tjungni
Apply suggestions from code review
97ae3ab 
Co-authored-by: Anton Reinhard <[email protected]>
@szabo137
Copy link
Member

szabo137 commented Nov 9, 2023

@AntonReinhard I replyed to all of your comments. Please approve, if you are happy with this 😺

@tjungni
Copy link
Member Author

tjungni commented Nov 9, 2023

@AntonReinhard I replyed to all of your comments. Please approve, if you are happy with this 😺

Thank you!

@szabo137 szabo137 merged commit 4bd0d32 into QEDjl-project:dev Nov 12, 2023
2 checks passed
@AntonReinhard AntonReinhard added this to the Release-0.1.0 milestone Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@szabo137 szabo137 szabo137 left review comments

@AntonReinhard AntonReinhard AntonReinhard approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Release-0.1.0
Development

Successfully merging this pull request may close these issues.
3 participants
@tjungni @szabo137 @AntonReinhard