-
Notifications
You must be signed in to change notification settings - Fork 17
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
Functional style plaquette #203
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Since this seems like a large change, please make the following updates:
|
Done
In my opinion, this would be better in the package documentation (see #109) |
Pinging for review, I would like this branch to be merged if you are still convinced that this is the right way to go. Feel free to comment on anything like @smburdick did, this is helpful to see if I missed anything. |
+1 to getting some help reviewing this!
…On Thu, May 2, 2024 at 2:04 AM Adrien Suau ***@***.***> wrote:
Pinging for review, I would like this branch to be merged if you are still
convinced that this is the right way to go. Feel free to comment on
anything like @smburdick <https://github.com/smburdick> did, this is
helpful to see if I missed anything.
—
Reply to this email directly, view it on GitHub
<#203 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAKAXTCSPMXV4HXHSYXU3VTZAH6R3AVCNFSM6AAAAABGAATQI6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOBZHE2TGOBRGU>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Goal of the PR
Per this comment from @inmzhang and the discussion/poll that followed it seems like the active developers in this repository prefer a functional-style rather than an inheritance based style. This means that instead of
with a
Plaquette
class whose sole purpose is to provide a common interface between plaquette, everyone voted for a functional-style:with
Plaquette
being the only class used, and sub-classes are replaced by functions that initialisePlaquette
instances.How was it done?
This change is mostly (entirely?) cosmetic, as the sub-classes of
Plaquette
were only initialising correctly theirPlaquette
subclasses. This shows quite well in this PR: the changes are quite small and can be summarised in 2 points:Plaquette
subclass, so this change can be done using a "dumb" tool (i.e., just search & replace, no need for code-aware tools).Plaquette
in the inheritance hierarchy (e.g.ZRoundedInitialisationPlaquette
->ZInitialisationPlaquette
->Plaquette
) for which thesuper().__init__
call should be replaced with their superclass corresponding function.