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

Support negative exponents in mul_2exp #458

Merged
merged 4 commits into from
Dec 2, 2023

Conversation

skirpichev
Copy link
Contributor

@skirpichev skirpichev commented Nov 28, 2023

We try to convert to unsigned long (for mpfr_mul_2ui) and
fallback to long on overflow (for mpfr_mul_2si).

Closes aleaxit#328
@codecov-commenter
Copy link

codecov-commenter commented Nov 28, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e34324a) 85.05% compared to head (eee6f31) 85.35%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #458      +/-   ##
==========================================
+ Coverage   85.05%   85.35%   +0.29%     
==========================================
  Files          49       49              
  Lines       11733    11714      -19     
  Branches     2204     2204              
==========================================
+ Hits         9980     9998      +18     
+ Misses       1753     1716      -37     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@skirpichev
Copy link
Contributor Author

This does make sense for div_2exp() as well. But I'm thinking about some code reorganization first.

Maybe it does make sense to add some GMPy_Integer_As??? utility function, that try conversion to unsigned long and then to long?

@skirpichev skirpichev marked this pull request as draft November 28, 2023 02:54
@casevh
Copy link
Collaborator

casevh commented Nov 28, 2023

This does make sense for div_2exp() as well. But I'm thinking about some code reorganization first.

Maybe it does make sense to add some GMPy_Integer_As??? utility function, that try conversion to unsigned long and then to long?

That would make sense. We need to include 64-bit values on Windows where long is only 32-bits. size_t and ssize_t could be used as the types.

@skirpichev
Copy link
Contributor Author

That would make sense.

Is there any potential use cases besides div_2exp()?

@casevh
Copy link
Collaborator

casevh commented Nov 28, 2023

Is there any potential use cases besides div_2exp()?

Those would be the most common. Almost any code that relies on bit positions or bit length could use size_t or ssize_t. See the bit indexing code in gmpy2_xmpz_misc.c for use of ssize_t.

@skirpichev skirpichev marked this pull request as ready for review November 30, 2023 02:41
@skirpichev
Copy link
Contributor Author

@casevh, I think this is ready for review.

size_t and ssize_t could be used as the types.

I came to GMPy_Integer_AsUnsignedLongOrLong() helper function. GMP only has *_si/ui functions to work with signed/unsigned long.

@skirpichev
Copy link
Contributor Author

CI failures are unrelated to this pr.

@casevh
Copy link
Collaborator

casevh commented Nov 30, 2023

I came to GMPy_Integer_AsUnsignedLongOrLong() helper function. GMP only has *_si/ui functions to work with signed/unsigned long.

Agreed. Looks good. I'll merge it this weekend / when you tell me it is ready.

@skirpichev
Copy link
Contributor Author

@casevh, it was mentioned above, that this pr is ready for review.

@skirpichev
Copy link
Contributor Author

@casevh, I've added yet another (last, I think ;)) commit to this PR, which utilizes new function in iroot() (to raise OverflowError instead of ValueError's on overflow).

@casevh casevh merged commit 5ad34f8 into aleaxit:master Dec 2, 2023
9 checks passed
@casevh
Copy link
Collaborator

casevh commented Dec 2, 2023

Thanks.

@skirpichev skirpichev deleted the fix-328-mul_2negexp branch December 2, 2023 08:47
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.

mul_2exp does not accept negative exponents TypeError in gmpy2.iroot
3 participants