Try first cut at issue 368 #371
Conversation
|
Hi @jzmaddock what's your opinion on this PR? I introduced rounding-up in selected areas only including Advantages include:
Disadvantage is that rounding has not yet been successfully applied to arithmetic operations and there are some open questions regarding the best way to do this. This could/should be completed within the framework of also refining equality on the sub-limb level. At the moment, however, there are no tangible disadvantages other than items that were already missing anyway. And CI is passing (except Circle CI has been spinning around for a day or so). So what do you think of this PR, its scope of change and if we should actually merge? |
test/git_issue_368.cpp
Outdated
| const std::string str_a = boost::lexical_cast<std::string>(a); | ||
| const std::string str_b = boost::lexical_cast<std::string>(b); | ||
|
|
||
| result_is_ok &= (str_a == str_b); |
There was a problem hiding this comment.
Questions: why are we comparing strings rather than values, and should we also test inputs which are not integer? Negative values as well, plus floor too I suspect ;)
There was a problem hiding this comment.
why are we comparing strings rather than values,
In this PR, there is rounding on floor, ceil and cast-to-int only. I removed the use of strings. But the comparison with, say, the result of floor or ceil is only true if the value being compared is casted to int prior to comparison. This is now done without strings in the candidate code of the PR's test file.
we also test inputs which are not integer
It seems like there are already-existing rounding tests which do this.
Negative values as well, plus floor too I suspect
Understood. Done in resent push(es).
| result = result.extract_integer_part(); | ||
| if(result.isint()) | ||
| { | ||
| ; |
There was a problem hiding this comment.
if(result.isint())
?
Indeed. These sequences have been simlified in recent push(es).
|
|
||
| local_limb_type tmp_limb_0 = data[0U]; | ||
|
|
||
| // Manually count the number of base-10 digits on the zero'th limb. |
There was a problem hiding this comment.
This is the bit that caused me to assume that correct round was next to impossible to do efficiently in this type, but maybe it's not so bad? I can't help but wonder if there are more efficient implementations - maybe binary search comparisons? Or can we even do table lookup based on the most-significant-bit? Just thinking out loud here....
There was a problem hiding this comment.
the bit that caused me to assume that correct round was next to impossible to do efficiently in this type
Well, it sure is proving to be unruly. But there are some new efficiency provisions.
- There was an existing binary search which had calculated the number of base-10 digits in the leading limb. This is used instead of a manual counting loop now.
- For the power-of-10, a special non-
constexprtable lookup has also been created for quick calculations of the power(s) of ten in similar matters.
|
Hi @jzmaddock I have attempted to address the review questions one-by-one. Personally, I am skeptical to introduce partial rounding on The questions I ask myself are:
I tend to think we are around 2 and 3. I would, however, treat this skeptically. This PR does deal with #368 and a subset of similar issues. But it seems like a partial treatment of rounding, and partial could be interpreted as wrong or less right than uniform truncation. I ran some tests with rounding on arithmetic operators, but found that this leads to needing quite a few changes in tolerances and behaviors that are already possibly running. I am not sure if rounding over all (also on arithmetic operations) would break existing code or not? I also came to believe that rounding only makes sense if we combine it with a more exact reatment of equality (equality on the sub-limb, digit level). This has also been avoided over the years. I have no problem keeping these changes in the PR or letting it sit for a while, or even dropping this effort. Although a lot of folks are happily using Thoughts? |
This PR attempts to address #368 with minimally non-intrusive changes of rounding in selected (but not all) places.