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

Adding RoundUp #194

Closed
wants to merge 3 commits into from
Closed

Adding RoundUp #194

wants to merge 3 commits into from

Conversation

da0x
Copy link

@da0x da0x commented Dec 5, 2020

Adding round up. Rounds decimal towards positive infinite.

@mwoss
Copy link
Member

mwoss commented Dec 7, 2020

Thanks, @da0x for your contribution. I will review ur PR tomorrow. I have to communicate with Jason (second contributor) as I know he has already started the implementation of RoundUp/Down methods.

decimal.go Outdated
// (1.52).RoundUp(1) => 1.6
// (1.1001).RoundUp(2) => 1.11
func (d Decimal) RoundUp(places int32) Decimal {
factor := NewFromFloat(10).Pow(NewFromInt(int64(places)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Decimal Pow is not a high-performance method. I have benchmarked them before but not have results here. You can try to benchmark it.

How about

factor := NewFromFloat(math.Pow(10, float64(-places))

Copy link
Author

Choose a reason for hiding this comment

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

Thank you! Great suggestion, I'll try it out.

Copy link
Author

Choose a reason for hiding this comment

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

using math.Pow:

BenchmarkDecimal_RoundUp-8                            	  799142	      1522 ns/op

using Decimal.Pow

BenchmarkDecimal_RoundUp-8                            	  255872	      4411 ns/op

I'll update the PR with your suggestion.

@mwoss
Copy link
Member

mwoss commented Dec 8, 2020

I was thinking for a bit about your implementation and I guess there is still a place for improvement. As we decided with Jason to include RoundUp/Down methods in the 1.3 release I would reiterate the implementation to be more performant.
I have already prepared the improved implementation for RoundUp, I think I will finish the implementation of RundDown tomorrow (don't have much time today tho :<)
We can collaborate on creating tests for those methods if you want :DD

Thanks a lot for your contribution and interest in decimal lib! :3

@da0x
Copy link
Author

da0x commented Dec 8, 2020

I agree, N*10^n can be easily optimized by shifting the decimal point instead of multiply/divide. I'm ok with this implementation though since performance isn't an issue for me. You can accept this PR and later replace it with your implementation or just keep it as a reference for others until yours is deployed. Up to you! Thanks for building this library guys. very useful.

Copy link
Contributor

@lovung lovung left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you so much.

@lovung
Copy link
Contributor

lovung commented Dec 9, 2020

Related with this issue #191

@mwoss
Copy link
Member

mwoss commented Dec 10, 2020

I apologize I still did not create PR with my solution. I had a lot of things to do today. I promise I will do it tomorrow, I will keep you posted :))

@da0x
Copy link
Author

da0x commented Dec 15, 2020

Resolved: #196 (comment)

@da0x da0x closed this Dec 15, 2020
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.

None yet

3 participants