-
Notifications
You must be signed in to change notification settings - Fork 112
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
cpp_dec_float_100 can only be initialized precisely from a string? maybe should be documented, or what am I doing wrong? #289
Comments
Your observation is correct and this is, in fact, the correct behavior.
Consider the examples below.
Here we find that integers like 12 can be represented exactly. Also fractions of liniear combinations of small
I believe it is, though let's find out if the docs are clear and overt enough on this matter.
We think it is best the way it is. You could, if you wanted, limit representation to the precision of built-in |
oh I didn't realize cpp_dec_float_100 had a string constructor (I skimmed docs, never saw it. I'm sure it's there but not obvious). my knee-jerk reaction is to say that since assigning from a non-string can lose precision that those constructor overloads/assignments should be deleted.... but then that would probably make the class pretty useless since now you can't do:
which seems like a perfectly normal use. if only there was a way to emit a warning when the user was initializing via hardcoded literal double, like so: I mean I get that the compiler is interpreting it as a double (at least that's my best guess as to what's happening), but it is still a gotcha to newcomers (I been working with Multiprecision for less than a day and it bit me for a decent chunk of time) |
This is a massive pit waiting for the unwary to drop into. There are numerous warning in the docs and example, but obviously never enough. And it will still bite you at some time in the future - been there, been bitten :-( . I have long wondered if a macro-controlled option might be feasible to warn about all uses of implicit conversion from double or float, at least in the constructor? So when people get unexpected results, they can switch this on as a debugging aid. The dilemma is that it is very inconvenient to write code if there is not an implicit constructor from double. |
libclang (a la clang-tidy) can detect and warn when initializing via a
hardcoded double literal... but I'm not sure if there's a way to
detect it during normal compilation
…On 1/15/21, Paul A. Bristow ***@***.***> wrote:
This is a massive pit waiting for the unwary to drop into. There are
numerous warning in the docs and example, but obviously never enough.
And it will still bite you at some time in the future - been there, been
bitten :-( .
I have long wondered if a macro-controlled option might be feasible to warn
about all uses of implicit conversion from double or float, at least in the
constructor? So when people get unexpected results, they can switch this on
as a debugging aid.
The dilemma is that it is very inconvenient to write code if there is not an
implicit constructor from double.
--
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
#289 (comment)
|
It seems like there is no direct action needed for this ticket. Potential enhancements include:
|
Sorry for being late to the party: I fear I'm with Chris on this one, there's nothing much we can actually do here without causing massive breakages (by banning all such conversions). In an ideal world we would:
|
I see no mention of this gotcha on this page of documentation: cpp_dec_float. And indeed the example at the bottom of that page encourages the "from numeric literal" approach of initialization. I dove straight into cpp_dec_float in a "get something working as fast as possible" mentality... So there is still an action to be taken: document it (better). Replace all uses of initialization in examples/docs to use the string versions... so that newcomers are less likely to get bitten. |
I just skimmed the Introduction doc and still saw no mention of this gotcha. I've never seen it mentioned anywhere except this bug report. |
Maybe we (or I) closed this issue too soon. Or maybe we should write a passage about initializing multiprecision float types in their constructors. I made a little graphic with some red arrows showing where such a section on Initializing Float Types or similar might make sense. (See image below.) Thoughts? |
I would like to move forward on this issue, but we need a decision to either close it with no Boost changes (but increased awareness) or to make a small addition to the docs along the lines above or similar. Thoughts? |
If we do decide to add a few lines to the docs, I will be happy to draft or write them. |
Adding an example with suitably eye catching title sounds good to me! |
Agrred. See #301. |
Fixes #289 with a clear intuitive example
That's definitely progress, but I still recommend updating all the existing docs/examples to use the string constructor. |
I feel rather strongly like it is not a one or the other decision. Developers are free to use construction or assignment from either string, built-in floating-point or integral typse, or from other multiprecision types. And exactly this situation is reflected in the documentation. So to answer your query, I do not see the need to pursue that idea you propose. |
I note that users (even expert C++ers, and not just me) keep falling into this big pit and quietly getting reduction to double. precision. This suggests that we can't have too many warnings about the risk. The results are very quiet and very hard to pinpoint. Increasing use of auto increases the risk further. I'd also still like the ability to temporarily 'switch off' the constructors from double etc, probably using a macro that users may defines to do this. Any compile errors that this provokes will highlight potential causes of loss of precision. |
I have done things like this in embedded micdrocontroller projects where developers might really not tolerate heavyweight components such as IO-streaming. I could imagine doing something like this here might increase confusion rather than decrease it? We could isolate the perfect and pure ctors and assignments of string(s) and integrals? Maybe @jzmaddock will weigh in, although there is really a lot going on now. My first reaction is that if we do this, then maybe in another issue? |
If we do end up pursuing this, I could imagine there might be a lot of edits needed in the Math library as well. |
The more I think about this, the less I like any further action. Consider:
Which is exactly the same trap that the OP fell into, and is clearly nothing to do with us. No compiler that I know of will warn on this either. Now consider making things stricter, if we make construction from floating point types explicit then:
Code like this is utterly ubiquitous in Boost.Math, and numerical code in general, and relies on an implicit conversion from type OK, so how about a warning? Well first off there is no good portable mechanism to actually do this, but on a practical note, the pages and pages of warnings and backtraces you would get from Boost.Math usage say, would be so problematic that we would rightly get a lot of complaints. The urge to "fix" things with some casts would be overwhelming, but as noted above, would actually make things worse! Something we do need to investigate more when we have time, is adding a) constexpr support for cpp_bin_float, and b) a reasonably generic mechanism for describing user-defined numeric literals, so to take Chris's example of 0.1 as a decimal string:
Would do the right thing, as would:
with the latter being inexact, and the former exact. And finally.... there is no "loss of precision" in converting from double to a multiprecision type. None, not one bit. In fact the conversion is absolutely exact as long as the target type is binary. In the OP's case the precision loss occurred prior to the library ever seeing the value as a result of the way the value was encoded as a double literal. I feel it's important to stress this lest folks get the wrong idea that the library somehow loses digits when converting from double, because it absolutely does not do so. Whew, and apologies if this came across as a bit of rant :) |
I find this conversation to be very valuable, in understanding when to construct multiprecision |
Agreed on this and all your other points. User-defined literals with suffixes might be the way to go. I get the feeling, nonetheless, that handling this issue may become easier in upcoming years as more and more language features and implementation flexibility becomes available in Boost.Multiprecision --- a long-term perspective. On this issue in the short-term, as it is unlikely that large-scale changes will be started soon, I would close it, let thoughts coalesce and table it for later. |
All this is absolutely correct and I am not arguing for a change to the provision of implicit constructors from built-ins. I am concerned that users are and will find it difficult to find where in their code they are losing precision because they are inadvertently constructing from a lower precision type. My suggestion to allow users to switch off temporarily construction from builtin is merely to help them diagnose their problem (or even potential problem) by flagging all the places where implicit construction is used. Or perhaps there is another way to highlight where reduction of precision may happen? |
That is a valid point. An easier, almost trivially outdated, method would be to support output of optional debug strings when a built-in floating-point constructor runs. But I would almost recommend developers do this locally when seeking this kind of problem. |
Just started using Boost.Multiprecision and struggled to get a cpp_dec_float_100 initialized to some hardcoded value. Eventually got it working once I parsed a string containing that hardcoded value. The docs didn't mention this I just sort of got lucky and deduced it.
Should this be documented?
Is there a better way to do it lol?
The text was updated successfully, but these errors were encountered: