| Issue 173: | Modulo operator does not work correctly for some floating point values | |
| 2 people starred this issue and may be notified of changes. | Back to list |
What steps will reproduce the problem?
evaluate -361.5 % 360 in haxe
What is the expected output? What do you see instead?
expected result should be -1.5
instead it is -721.5
What version of the product are you using? On what operating system?
haxe nightly/hxcpp 2.09 (I checked hx::DoubleMod unchanged in svn vs. local)
win64
Please provide any additional information below.
I was able to address this by changing the following in Math.cpp :
double DoubleMod(double inLHS,double inRHS)
{
if (inRHS==0) return 0;
return fmod(inLHS,inRHS);
/* double divs = inLHS/inRHS;
double lots = divs<0 ? ::floor(-divs) : ::floor(divs);
return inLHS - lots*inRHS; */
}
I don't know c++ outside of my ability to generate and compile via haxe (for which I thank you!) so I don't understand why you didn't use the native floating point modulo function here - it seems too obvious to be overlooked, so I can only assume it is some form of optimization? Making this change seems to now give results consistent with swf, javascript and neko. Floating point modulo operations are not include in the haxe unit tests despite being supported across the original 3 haxe targets for the '%' operator. PHP also fails with the above example, but for a different reason, no attempt is currently made to use a floating point modulus in the gerenated code...it currently uses the integer modulo operator internally, so changing to fmod in the php implementation addresses this inconsistency too (I will report this for the php target directly on haxe issues).
May 10, 2012
#1
greg.d...@gmail.com
May 10, 2012
Please see point #1 in comment 2 here for another related point : https://code.google.com/p/haxe/issues/detail?id=830#c2
May 11, 2012
Please accept a patch for review. And refer to https://code.google.com/p/haxe/issues/detail?id=830#c4 for related information. You may want to discuss this with Nicolas and Franco. I still don't know if you are not using fmod for a specific reason in the DoubleMod implementation, but I think I found a way to calculate it correctly without using fmod (although my code could perhaps be simplified a bit more). For the zero divisor check which returns 0 that you had in DoubleMod I changed the integer implementation to do it that way and for the floating point version to return NaN which is consistent with flash and javascript and neko (and php if my patch for php is accepted). So there are two implementations in the patch... one manually coded and the other using fmod (commented out). Both appear to work the same across a range of tests.
May 13, 2012
I forgot the modulo assignment operator support. The patch here replaces the above and addresses that as well I believe. Please give this a good deal of scrutiny as -I'll be honest- I really don't know c++. I took a real guess for the pass by reference syntax in Operators.h.
May 16, 2012
Thanks, I will have a look at this.
Status:
Accepted
Jun 2, 2012
I'll just use fmod - complies with neko. I've made the changes to svn, thanks for your work on this. Hugh
Status:
Fixed
Jun 2, 2012
Thanks! I also saw you added some extra tests. Please note there are a lot of modulo tests waiting for this in : https://code.google.com/p/haxe/issues/attachmentText?id=830&aid=8300009000&name=phpModulo3.patch
Jun 2, 2012
I see, you went for the vanilla fmod implementation, which will likely fail some of the tests I proposed. I'm afraid I'm not sure what the overall goals are in terms of consistency across haxe targets vs. performance for things like this. The patch I submitted (and the php one) were aiming predominantly for greater consistency across the various targets for integer and floating point modulo ops.
Jun 2, 2012
I think we can ignore the infinity cases, IMHO it's such a borderline scenario that it's not worth any performance loss in the more general cases.
Jun 3, 2012
Of course, if the documentation says "the result is undefined when the right-hand-side is zero", then we can all be right.
Jun 3, 2012
One more follow up on this from me. Sorry Hugh and others if it looks like I'm dragging it out, please bear with me and give this a couple of minutes' attention, I won't post anything this long again.
I freely accept Simon's point about the infinity cases. But I would request that you reconsider that the crashing scenario is something you'd want to avoid for cpp. I'm not sure whether X % 0 should fail gracefully (returning 0) or throw an exception like with neko... but it does fail gracefully on swf9 so that is what I drew from originally. I assumed it would be one of those things that should remain consistent. For Float modulo you have Math.NaN as a return option (which fmod does), for Integer modulo its obviously not so easy.
If you all decide that returning zero is not right for % 0 and throwing an exception is more correct, then we should also remove similar code (and perhaps the infinity checks as well) from the php patch I submitted. I am trying to aim for as much consistency (with minimal performance impact) as I can ask for here (or anywhere). This is obviously not for me personally, I can code around it, but because I think it's better for Haxe. Whatever the outcome, I'm prepared to contribute to the docs for this once its established to make it clearer for others.
How costly is it?
I ran some loop tests for the int div by zero scenario. On my machine at least, the cost is very slight. It amounts to an average of about 27 ms over tests of 10 million iterations (I just used 5%3 for testing). This was about 1.6% slower than not checking for zero in int modulo. The variation was such that the fastest test of the slower approach and the slowest test of the faster approach' times overlapped slightly. I won't claim that my testing was gold-plated robust, but I did give it a whirl. My averages were (for 5 tests each of 1e7 iterations) 1.680s for the current implementation and 1.707s for including the RHS parameter check.
Also I discovered that the modEq really needs to be channelled through a native integer modulo op if possible for integers instead of through the DoubleMod (fmod) approach. Although modEq is probably quite a rare use, fmod is quite a lot slower than using int modulo (2.8 secs using DoubleMod vs. 1.8secs using the int version below). Regardless, of whether you choose to implement the RHS zero check or not I think you might want to add the int version of ModEq because of this and because it also now behaves differently from the regular int Mod op in this regard (I now get a large negative Int for div by zero with a%=0).
Here's what I used for the int modulo in Operators.h:
#if !defined(_MSC_VER) || _MSC_VER > 1399
inline int Mod(int inLHS,int inRHS) { return inRHS==0 ? inRHS : inLHS % inRHS; } //tested against current version approx 1.6% slower
inline int ModEq(int &inLHS,int inRHS) { return inLHS = (inRHS==0)? 0: inLHS % inRHS;} //tested against current version (DoubleMod), this is much faster
#endif
Lastly - my earlier caveat about the above c++ code applies. I can tweak what I see in your code already but I could not write it myself. So if what I've done (which is not very much at all!) doesn't make sense, that would be the reason why :). You made it! Thanks for your patience!
|