-
Notifications
You must be signed in to change notification settings - Fork 15
Adaptive (n, 1, p)-QRACs #31
Adaptive (n, 1, p)-QRACs #31
Conversation
There are (a lot of) changes in this pull request that are unrelated to its stated goal (and provided without justification). A good pull request should make a single logical change, ideally the minimal changes required to meet its goal, as smaller pull requests are much easier to review than longer ones. That said, some of the suggested spelling substitutions might be improvements and should be considered separately, perhaps discussed in an issue first. I think it's worth saving this branch aside as is to keep a record. Any changes to interfaces, though, will break existing users of the code, so should be carefully considered, and if possible, the old names should be made to continue to work, keeping with the deprecation policy. |
I'm going to close this, since it is being superseded by other PRs. There's a lot of good stuff in here, but we're going to make each change one at a time instead. It will nonetheless be useful to reference this from time to time. |
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.
I added a few comments on the changes to encoding.py
. It would be nice to have a small PR that does a similar refactor, in preparation for adaptive QRACs.
has_even_parity = sum(dvar_values) % 2 | ||
has_even_count = num_dvars % 2 |
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.
I believe these should actually be called has_odd_parity
(or simply parity
) and has_odd_count
.
for i in range(num_dvars - 1) | ||
] | ||
) | ||
state = One if (has_even_parity if has_even_count else dvar_values[0]) else Zero |
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.
The double inline if
makes this line a bit obscure. I wonder if there's a more clear way to phrase this.
Summary
Details and comments