Skip to content

Comments

Separated integers from rationals and added simple FastInteger wrapper#751

Draft
Tomaqa wants to merge 4 commits intomasterfrom
separate_integer
Draft

Separated integers from rationals and added simple FastInteger wrapper#751
Tomaqa wants to merge 4 commits intomasterfrom
separate_integer

Conversation

@Tomaqa
Copy link
Member

@Tomaqa Tomaqa commented Aug 21, 2024

Existing FastRational class contained some functions that are not related to rationals but to integers.
I fixed this by making a derived class FastInteger and moving these specific functions there.

@Tomaqa Tomaqa requested a review from blishko August 21, 2024 19:05
@Tomaqa Tomaqa force-pushed the separate_integer branch 2 times, most recently from a02c3cc to 5569e3c Compare August 22, 2024 07:18
@Tomaqa
Copy link
Member Author

Tomaqa commented Aug 22, 2024

I also corrected arithmetic operations s.t. they return FastInteger and not FastRational.

@Tomaqa Tomaqa force-pushed the separate_integer branch 2 times, most recently from 64d1971 to dd14374 Compare August 23, 2024 14:42
@Tomaqa
Copy link
Member Author

Tomaqa commented Aug 23, 2024

I also experimented with a clearer distinction between Real and Integer, but it turned out to require quite a lot of work, so I gave up. The current solution partially divides the usage of the types, but it still uses integer operations on rational arguments while using static_cast on them. This is (currently) safe and the performance should be the same as before. But the current solution requires to explicitly cast/convert the arguments to integers, making it clearer what is going on.

@Tomaqa Tomaqa force-pushed the separate_integer branch 2 times, most recently from 1a2c4ad to 13adfbd Compare November 4, 2024 18:54
@Tomaqa Tomaqa marked this pull request as draft November 5, 2024 11:46
@Tomaqa Tomaqa force-pushed the separate_integer branch 4 times, most recently from a986a8a to d347179 Compare November 7, 2024 15:12
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.

1 participant