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

Log10 #544

Merged
merged 9 commits into from
May 10, 2024
Merged

Log10 #544

merged 9 commits into from
May 10, 2024

Conversation

ckormanyos
Copy link
Collaborator

No description provided.

Comment on lines 71 to 75
while(g > one)
{
// TODO(ckormanyos) Do we really have to multiply individual powers
// of 10 here? Or is there a reliable way to simply use pow10()?
// For instance count intagral orders of exp10val and divide by pow10().
Copy link
Member

Choose a reason for hiding this comment

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

The significand is normalized before being returned by frexp10 so you can divide by pow10(digits10 - 1) to avoid the loop

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

significand is normalized before being returned by frexp10 so you can divide by pow10(digits10 - 1) to avoid the loop

OK @mborland cool but now I'm stuck on another point. Do you know how to recognize a pure power of $10$ without burning too much CPU cycling?

Copy link
Member

@mborland mborland May 9, 2024

Choose a reason for hiding this comment

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

You could use detail::remove_trailing_zeros and compare the trimmed_number member to 1. That is about as efficient as it's going to get I think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Got it. Will commit cool reaction to advice.

Copy link

codecov bot commented May 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.6%. Comparing base (6820459) to head (6d5abe2).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           develop    #544     +/-   ##
=========================================
- Coverage     98.8%   98.6%   -0.1%     
=========================================
  Files          189     194      +5     
  Lines        11700   11525    -175     
  Branches         0    1380   +1380     
=========================================
- Hits         11556   11361    -195     
- Misses         144     164     +20     
Files Coverage Δ
include/boost/decimal/detail/cmath/log.hpp 100.0% <100.0%> (+4.2%) ⬆️
include/boost/decimal/detail/cmath/log10.hpp 100.0% <100.0%> (ø)
test/test_log.cpp 100.0% <ø> (ø)
test/test_log10.cpp 100.0% <100.0%> (ø)

... and 67 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6820459...6d5abe2. Read the comment docs.

@ckormanyos
Copy link
Collaborator Author

Got it. Will commit cool reaction to advice.

OK Matt (@mborland) I finally was able to implement a log10() with which I'm mostly satisfied. The newer stuff like trimming trailing zeros is very nice and convenient to work with.

I did notice, however, that there are several places in the code where we end up using triple-conditional type-definitions in order to arrive at uint32_t, uint64_t or the specialized 128-bit representaion type. Do you think it makes sense to collect these and support one single definition of the decimal-type's representation_type?

@mborland
Copy link
Member

Got it. Will commit cool reaction to advice.

OK Matt (@mborland) I finally was able to implement a log10() with which I'm mostly satisfied. The newer stuff like trimming trailing zeros is very nice and convenient to work with.

I did notice, however, that there are several places in the code where we end up using triple-conditional type-definitions in order to arrive at uint32_t, uint64_t or the specialized 128-bit representaion type. Do you think it makes sense to collect these and support one single definition of the decimal-type's representation_type?

I guess it would be easy to add the same typedef into each of the classes and just query that. I'm cool with it.

This was referenced May 10, 2024
Closed
@ckormanyos ckormanyos merged commit 641435d into develop May 10, 2024
61 checks passed
@ckormanyos ckormanyos deleted the log10 branch May 10, 2024 10:05
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