Community
Participate
Working Groups
The motivation for this request is twofold: First, the OCL-Ecore implementation should treat java.math.BigDecimal as a Real type. Second, even if it did, the current implementation cannot perform comparison operations on certain BigDecimal instances. Actually, the long story is that in my non-Ecore OCL Environment I already treat BigDecimals as Reals, so I had to fight the second problem first, but I digress... ;-) Please find attached three patches, which provide the following: * org.eclipse.ocl.ecore.patch => Makes BigDecimal an OCL Real type. * org.eclipse.ocl.ecore.tests.patch => Enhanced test cases for BigDecimal comparison. These, however, fail in most cases. * org.eclipse.ocl.patch => Fix for NumberUtil.isDouble(BigDecimal). Now the tests are green. Feel free to debug the failing testcases by setting a breakpoint in NumberUtil.isDouble(BigDecimal) before applying the third patch. The underlying problem is that BigDecimal("1.0") is not equal to BigDecimal("1"), which is true (in a weird way), but nonetheless screws up the isDouble() computation. BigDecimal("1.0") undoubtedly _is_ a real number, and can safely be converted to a double value. The implementation provided in the patch is closer to what I assume was the original intent of this method: Filtering out values that are beyond java.lang.Double's precision. If that was _not_ the original intent, you can simply return BigDecimal.doubleValue() every time and save the call to isDouble(). But I am still not totally happy, even with the fixed implementation. There's a saying that goes, "If you want the right results slowly, use BigDecimal, if you want the wrong resuts fast, use double". While debugging the code, I have seen BigDecimals of "1.1" converted to a 1.100000000000016 double value. I can see wrong comparison results looming on the horizon... Maybe we should open another enhancement request to work with BigDecimals internally instead of Double. But for now, I'd be more than happy if the patches could be applied not only in the 1.3 stream, but also for the 1.2.x stream.
Created attachment 111431 [details] Makes BigDecimal Real
Created attachment 111432 [details] Tests BigDecimal comparison
Created attachment 111433 [details] Fixes NumberUtil.isDouble()
Created attachment 111434 [details] Makes BigDecimal Real Sorry, posted wrong file the first time.
Hi, Achim, As this is an enhancement (not just in Bugzilla, but in fact), it would not be appropriate to implement it in the 1.2.x maintenance branch, which is only for bug fixing. It would be a good idea to raise a separate enhancement request for using BigDecimal instead of Double for internal calculations; the performance impact suggests that it should be a (non-default) user Option. Yes, the purpose of NumberUtil.isDouble() is to detect whether a number is in the double range so that it can be narrowed to that type. The interpreter aims to always return numeric values in the lowest position that will handle them.
Created attachment 112213 [details] Consolidated patch Consolidated the three patches into one. Achim, a tip: it is much less tedious for both the contributor and the committer reviewing a patch to create a single Eclipse-workspace patch containing all of the changes, by multi-selecting the affected files or projects. Otherwise, anybody applying the patches has to open each one to see which file is affected and then select that file in the Apply Patch wizard.
Hi, Achim, The patch mostly looks good to me. I especially like the JUnit tests; thanks for that! I have a couple of questions: I assume that underflows in the BigDecimal-to-double conversion simply result in a 0.0 double, so that BigDecimals of very small magnitude would be interpreted as being convertible to Double, which is not really true. I am not sure how much of a problem this is, but it should be possible to detect an underflow in the NumberUtil using the algorithm that I had implemented, previously. Some kind of hybrid approach may be in order. Also, I am hesitant to implement BigDecimal support without BigInteger support. Would you volunteer to add that? Finally, in looking at the UML binding, I noticed that we make no attempt on that side to re-interpret the Ecore Primitive Types defined by MDT UML2 as OCL Real/Integer/etc. I would probably do that once your patch for Ecore is complete.
Hi Christian, Sorry for the three patches. I didn't know that this is less convenient. I'll provide one consolidated patch in the future. > I assume that underflows in the BigDecimal-to-double conversion simply result > in a 0.0 double, so that BigDecimals of very small magnitude would be > interpreted as being convertible to Double, which is not really true. I am not > sure how much of a problem this is, but it should be possible to detect an > underflow in the NumberUtil using the algorithm that I had implemented, > previously. Some kind of hybrid approach may be in order. I don't think an underflow is very different from the general loss of precision we may encounter when converting BigDecimals to double. As long as we're using double values internally, we will always have to deal with glitches in precision. See this, for example: http://java.sun.com/j2se/1.5.0/docs/api/java/math/BigDecimal.html#BigDecimal(double) What I'm trying to say is: From my point of view it doesn't matter that the BigDecimal to double conversion cannot be guaranteed to be exact, when we have an inherent lack of precision in doubles anyway. > > Also, I am hesitant to implement BigDecimal support without BigInteger support. > Would you volunteer to add that? > If you mean, "do the same thing you've done for BigDecimals again for BigIntegers", yes, I can do that. Aytually, I'll do it right away, because if I don't, it would have to wait until mid-October... I have one question for you, too: Would you consider the current NumberUtil.isDouble() behavior buggy? I mean, just isDouble() in itself, not regarding the fact that ocl.ecore currently isn't able to handle BigDecimals? Since I'm not using ocl.ecore but my own OCL environment, which _is_ capable of dealing with BigDecimals, this method is actually called and produces, let's say, surprising results. So this really _is_ a bug from my point of view, not an enhancement request. It your call, of course, but if you agree to that reasoning, I could open a separate bug report for just the NumberUtil.isDouble() fix for the 1.2.x stream.
(In reply to comment #8) > Hi Christian, > > Sorry for the three patches. I didn't know that this is less convenient. I'll > provide one consolidated patch in the future. No problem. I don't mean to whine. ;-) > I don't think an underflow is very different from the general loss of precision > we may encounter when converting BigDecimals to double. As long as we're using > double values internally, we will always have to deal with glitches in > precision. See this, for example: > > http://java.sun.com/j2se/1.5.0/docs/api/java/math/BigDecimal.html#BigDecimal(double) > > What I'm trying to say is: From my point of view it doesn't matter that the > BigDecimal to double conversion cannot be guaranteed to be exact, when we have > an inherent lack of precision in doubles anyway. Right, but what the NumberUtil was attempting to do was to determine whether the BigDecimal could be converted to a Double without loss of precision. The NumberUtil's job is to find the most efficient representation when possible. However, since it seems likely that a BigDecimal is almost never exactly convertible to a Double, this doesn't seem like a useful goal. NumberUtil should really be attempting to narrow values down to the precision in which the interpreter does its calculations. This would argue for accepting the precision errors if Real values are Doubles, and keeping BigDecimals if real values are BigDecimals (hypothetical user option). Otherwise, I expect that your BigDecimal-based computation simply always fails, because the interpreter doesn't know what to do with BigDecimals. > > Also, I am hesitant to implement BigDecimal support without BigInteger support. > > Would you volunteer to add that? > > > > If you mean, "do the same thing you've done for BigDecimals again for > BigIntegers", yes, I can do that. Aytually, I'll do it right away, because if I > don't, it would have to wait until mid-October... That's exactly what I mean. :-) Thanks! > I have one question for you, too: Would you consider the current > NumberUtil.isDouble() behavior buggy? I mean, just isDouble() in itself, not > regarding the fact that ocl.ecore currently isn't able to handle BigDecimals? If by current you mean the state of the 1.2 release, then I think, yes. It seems that it would almost never return a true result. It is also at odds with the documented purpose of NumberUtil::higherPrecisionNumber > Since I'm not using ocl.ecore but my own OCL environment, which _is_ capable of > dealing with BigDecimals, this method is actually called and produces, let's > say, surprising results. So this really _is_ a bug from my point of view, not > an enhancement request. > > It your call, of course, but if you agree to that reasoning, I could open a > separate bug report for just the NumberUtil.isDouble() fix for the 1.2.x > stream. Sounds like a good idea to me. Actually supporting BigDecimals as Reals in OCL is an enhancement, but NumberUtil is inconsistent with itself.
Created attachment 112222 [details] Consolidated BigDecimal + BigInteger patch This patch contains the BigDecimal fixes from the previous patches, plus support for BigInteger. For whatever reason Bugzilla does not allow me to make the previous patch obsolete.
Added bug #246892 for the NumberUtil.isDouble() fix for 1.2.x. Added bug #246895 for the BigDecimal computation option. Anything else I can do for you in this area?
(In reply to comment #11) > Added bug #246892 for the NumberUtil.isDouble() fix for 1.2.x. > > Added bug #246895 for the BigDecimal computation option. > > Anything else I can do for you in this area? Heh, heh. No, thanks for your contribution! I have committed your Ecore patch for Big{Decimal,Integer} support, including the NumberUtil fix. I have also updated the UML binding to support the numeric, boolean, and string primitive types in the MDT UML2 Ecore Primitive Types library. That was a long-needed enhancement. Thanks for making it so easy! (I copied your JUnit test cases and adapted them for UML)
You're very welcome. :)
Closing after over 18 months in resolved state.
