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

Added pow function fpm #1621

Closed
wants to merge 1 commit into from
Closed

Conversation

Zain-Balkhi
Copy link

Implemented a potential working version of pow using fixed point math. No way to test because project won't run.

Implemented a potential working version of pow using fixed point math. No way to test because project won't run.
Copy link
Member

@heinezen heinezen left a comment

Choose a reason for hiding this comment

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

Hey thanks for the contribution, but your code still contains floating point math which is not what #1543 was about. What we want are fuctions that implement these operations by only using fixed-point arithmetic.

I don't know if you want to give this another try but we can't really use this code right now.

Comment on lines +595 to +597
if (base == 0.0 && exponent == 0.0) {
return openage::util::FixedPoint<I, F>::quiet_NaN(); // Undefined; return NaN
} else if (base == 1.0 || exponent == 0.0) {
Copy link
Member

Choose a reason for hiding this comment

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

why compare to doubles in the if conditions when we could compre to fixed point values instead?

Comment on lines +600 to +601
double log_base = std::log(base.get_raw_value());
double result = exponent * log_base;
Copy link
Member

Choose a reason for hiding this comment

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

The pure fixed-point math functions should avoid any floating point arithmetic. Avoiding the floating point rounding erros is the point of using fp math after all.

Copy link
Member

Choose a reason for hiding this comment

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

this file would need to be removed

Copy link
Member

Choose a reason for hiding this comment

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

and this one

Copy link
Member

Choose a reason for hiding this comment

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

and this one

Copy link
Member

Choose a reason for hiding this comment

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

and this one

@heinezen
Copy link
Member

I'm closing this because this is not what we are looking for.

@heinezen heinezen closed this Mar 31, 2024
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.

2 participants