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

64 bit signed arithmetic support #57

Merged
merged 13 commits into from
May 16, 2024
Merged

64 bit signed arithmetic support #57

merged 13 commits into from
May 16, 2024

Conversation

qartik
Copy link
Collaborator

@qartik qartik commented Apr 23, 2024 •
edited

Finish BinArray2 with 64-bit signed as the default type and migrate to it from BinArray.

Also, add a unit test file to expose interesting edge cases.

@qartik qartik changed the base branch from master to development April 23, 2024 20:44
@qartik qartik marked this pull request as ready for review April 25, 2024 16:57
@qartik qartik requested a review from ciaranra as a code owner April 25, 2024 16:57
@qartik qartik requested a review from qciaran April 25, 2024 16:58
qartik
qartik commented Apr 25, 2024
View reviewed changes
tests/unit/test_binarray.py Outdated Show resolved Hide resolved
dlucchetti
dlucchetti reviewed May 2, 2024
View reviewed changes
Copy link

@dlucchetti dlucchetti left a comment

Choose a reason for hiding this comment

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

Seems reasonable. Not 100% sure on the issue around masking width. Seems that it may be best to integrate this to get some broader testing. I believe it should only impact QIR.

python/pecos/classical_interpreters/phir_classical_interpreter.py Outdated Show resolved Hide resolved
@qartik
feat: partition data_types into signed and unsigned
5829138 
also make size optional for signed integer type
qartik
qartik commented May 4, 2024
View reviewed changes
ruff.toml Outdated Show resolved Hide resolved
@qartik qartik requested a review from dlucchetti May 6, 2024 17:35
qartik
qartik commented May 15, 2024
View reviewed changes
pyproject.toml Outdated Show resolved Hide resolved
qciaran
qciaran requested changes May 16, 2024
View reviewed changes
Copy link
Collaborator

@qciaran qciaran left a comment

Choose a reason for hiding this comment

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

I tried some examples myself. Everything worked and the code makes sense.

So LGTM except I think we should keep to 0.6.0.dev* and I will try to work on getting to a release sooner than later.

pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@qartik @qciaran
Fix: version to dev2
8849f6b 
Co-authored-by: Ciarán Ryan-Anderson <70174051+qciaran@users.noreply.github.com>
@qartik qartik requested a review from qciaran May 16, 2024 19:03
qciaran
qciaran approved these changes May 16, 2024
View reviewed changes
Copy link
Collaborator

@qciaran qciaran left a comment

Choose a reason for hiding this comment

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

LGTM

ciaranra
ciaranra approved these changes May 16, 2024
View reviewed changes
Copy link
Member

@ciaranra ciaranra left a comment

Choose a reason for hiding this comment

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

LGTM

@qartik qartik merged commit 5aaa88c into development May 16, 2024
9 checks passed
@qartik qartik deleted the 64-bit-signed-arith branch May 16, 2024 19:19
@qartik qartik mentioned this pull request May 31, 2024
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ciaranra ciaranra ciaranra approved these changes

@qciaran qciaran qciaran approved these changes

@dlucchetti dlucchetti Awaiting requested review from dlucchetti

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@qartik @ciaranra @dlucchetti @qciaran