lucenenet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Christopher Currens <currens.ch...@gmail.com>
Subject Re: lucenenet git commit: use proper float comparison
Date Sun, 31 May 2015 06:33:05 GMT
I was able to confirm that the 32-bit and 64-bit JVMs both emit code using
SSE. So maybe there is something there, or maybe not.

It's weird though, because if I run the test over and over (using the NUnit
adapter in visual studio, so x86) it sometimes passes, and I'm not sure
why. You are right, though, it is something related to the conversion
between float and double. Every time it fails, I output the roundtrip
string for both skipToScore and scorer_.Score() as floats and then casted
as double. Every single time when it fails, the float values are exactly
the same and those same float values casted to doubles produce different
numbers. I mean, this is what you saw yourself in the tests, I'm just here
to confirm I'm seeing the same thing (and it's puzzling).

I feel like this one is out of our control (maybe a .NET bug?) and maybe
the best fix is to to do what you've already done and avoid the conversion
to double altogether via Assert.IsTrue.

-Christopher


On Sat, May 30, 2015 at 9:03 PM, Christopher Currens <
currens.chris@gmail.com> wrote:

> The .NET jitter emits different code to handle floating point instructions
> in x86 vs x64. At least on my machine, I noticed that the native assembly
> code generated by the jitter when running in x86 uses the x87 extensions
> for floating point and in x64 it uses SSE. I believe that this is only an
> issue when dealing with single-precision floating point numbers, which are
> used pretty much everywhere in search. The reason is because the x87
> extensions, by default, use 80-bit double-extended precision internally
> (thanks, Wikipedia!) whereas x64 uses single-precision instructions (and
> thus the mantissa is truncated) which means we'll get different results
> between the two architectures.
>
> Resharper defaults to x64. If I use the NUnit Test Adapter and run the
> unit tests using visual studio directly, which runs in 32-bit mode, I can
> get the tests to fail almost all the time.
>
> This is a good catch. I'm not sure if we should change nunit to be x64
> necessarily. It's possible that this is exposing a real code issue
> somewhere, or at least an inconsistency in behavior between .NET and Java.
> I think I might pull down the java code and see if there's a difference in
> this test between a 32-bit and 64-bit JVM. I don't know what kind of
> assembly instructions that are emitted by Java's jitter.
>
> -Christopher
>
> On Sat, May 30, 2015 at 6:47 PM, Laimonas Simutis <laimis@gmail.com>
> wrote:
>
>> FINALLY I am able to reproduce it locally. Looking through TC build I
>> noticed this:
>>
>> Running NUnit-2.6.3 tests under .NET Framework v4.0 x86
>>
>> Note x86... So instead of running test via Resharper and built in NUnit, I
>> ran it  with nunit 2.6.3 via command line. Tests fail with the odd float
>> issues if I run it with nunit-x86, and pass if I run it with nunit.exe
>> (both version 2.6.3). I am on a 64 bit machine, and so are the TC build
>> agents it seems.
>>
>> I am still not sure why this causes the failures to occur, but do we need
>> to adjust what nunit build we use to run the tests?
>>
>> On Sat, May 30, 2015 at 4:28 PM, Laimonas Simutis <laimis@gmail.com>
>> wrote:
>>
>> >
>> >
>> > On Sat, May 30, 2015 at 4:01 PM, Itamar Syn-Hershko <itamar@code972.com
>> >
>> > wrote:
>> >
>> >> And when you refactor _scorer.Score() to be in a different line it
>> passes
>> >> 100% of the time on all platforms? that doesn't sound right.
>> >>
>> >
>> > It continues to pass on mine (I can never get those to fail locally),
>> and
>> > ran the test several times on TC and it passed. I know, it sounds odd,
>> I am
>> > at a loss to explain it.
>> >
>> >
>> >>
>> >> Also, not in front of VS now, but AreEquals should already be doing
>> this
>> >> epsilon thing no?
>> >>
>> >
>> > That's what I thought too. The only odd thing there is no "float"
>> overload
>> > and only "double" so not sure if conversion from float to double might
>> be
>> > introducing rounding issues here too. That's why I replaced it with
>> epsilon
>> > just to see what would happen and it still failed so then I went with
>> > precalculating scorer_.Score() before comparison just to see what would
>> > happen.
>> >
>> > And check this out. I put the comparison back like it used to be
>> > (Assert.AreEquals) and wrapped in catch to output to console the values:
>> >
>> > float skipToScore = scorer_.Score();
>> > try
>> > {
>> >     Assert.AreEqual(skipToScore, scorer_.Score(), MaxDiff, "unstable
>> > skipTo(" + i + ") score!");
>> > }
>> > catch (AssertionException ex)
>> > {
>> >     Console.WriteLine("Failed, these two were deemed not equal:");
>> >     Console.WriteLine(skipToScore.ToString("R"));
>> >     Console.WriteLine(scorer_.Score().ToString("R"));
>> >     throw;
>> > }
>> >
>> > Look at the output on TC:
>> >
>> > Test(s) failed.   unstable skipTo(3) score!
>> >   Expected: 115019.984375d +/- 0.0010000000474974513d
>> >   But was:  115019.98828125d
>> >
>> > ------- Stderr: -------
>> > Failed, these two were deemed not equal:
>> > 115019.984
>> > 115019.984
>> >
>> > You can see how the floats were converted to doubles and furthermore how
>> > when I call Score() in catch section, it returns 115019.984 yet when it
>> was
>> > called in Assert it is outputting 115019.98828125d. and 0.988 and is off
>> > from 0.984 by more than 0.001 (which is the value of MaxDiff).
>> >
>> >
>> >
>> >
>> >>
>> >> --
>> >>
>> >> Itamar Syn-Hershko
>> >> http://code972.com | @synhershko <https://twitter.com/synhershko>
>> >> Freelance Developer & Consultant
>> >> Lucene.NET committer and PMC member
>> >>
>> >> On Sat, May 30, 2015 at 10:46 PM, Laimonas Simutis <laimis@gmail.com>
>> >> wrote:
>> >>
>> >> > Itamar,
>> >> >
>> >> > These float comparison are killing me :) I am pretty sure all the
>> >> remaining
>> >> > failures in core tests are related to float issues.
>> >> >
>> >> > I am trying to use epsilon here by changing
>> >> >
>> >> > AreEqual(skipToScore, scorer_.Score(), MaxDiff) to
>> >> > IsTrue(Math.Abs(skipToScore - scorer_.Score()) < MaxDiff).
>> >> >
>> >> > It is similar to the link you provided except I am not
>> >> > handling infinite and values close to 0, which are not expected and
>> do
>> >> not
>> >> > occur in this test.
>> >> >
>> >> > I can get this test to pass by taking out scorer_.Score() calculation
>> >> and
>> >> > calculating it separately and then comparing, like this:
>> >> >
>> >> > var secondScore = scorer_.Score();
>> >> > IsTrue(Math.Abs(skipToScore - secondScore) < MaxDiff).
>> >> >
>> >> > In this case, the scorer_.Score() is doing a bunch of float adds  /
>> >> > multiplies (
>> >> >
>> >> >
>> >>
>> https://github.com/apache/lucenenet/blob/master/src/Lucene.Net.Core/Search/DisjunctionMaxScorer.cs#L58
>> >> > )
>> >> > so I can see where rounding error could come in but still cannot
>> explain
>> >> > how it consistently fails on some env and not the others. Also have
>> no
>> >> idea
>> >> > how to proceed with this issue besides changing the order of
>> >> calculations,
>> >> > like I did with the above to get it to pass. Just don't feel
>> confident
>> >> that
>> >> > there is no bigger issue somewhere else.
>> >> >
>> >> >
>> >> > Laimis
>> >> >
>> >> >
>> >> >
>> >> > On Sat, May 30, 2015 at 2:56 PM, Itamar Syn-Hershko <
>> itamar@code972.com
>> >> >
>> >> > wrote:
>> >> >
>> >> > > Float comparison is not as trivial - you should probably use
>> epsilon
>> >> --
>> >> > see
>> >> > > http://stackoverflow.com/a/3875619/135701 for example
>> >> > >
>> >> > > --
>> >> > >
>> >> > > Itamar Syn-Hershko
>> >> > > http://code972.com | @synhershko <https://twitter.com/synhershko>
>> >> > > Freelance Developer & Consultant
>> >> > > Lucene.NET committer and PMC member
>> >> > >
>> >> > > On Sat, May 30, 2015 at 9:50 PM, <laimis@apache.org> wrote:
>> >> > >
>> >> > > > Repository: lucenenet
>> >> > > > Updated Branches:
>> >> > > >   refs/heads/failingtests bdf2899a0 -> 6a81f8606
>> >> > > >
>> >> > > >
>> >> > > > use proper float comparison
>> >> > > >
>> >> > > >
>> >> > > > Project: http://git-wip-us.apache.org/repos/asf/lucenenet/repo
>> >> > > > Commit:
>> >> > http://git-wip-us.apache.org/repos/asf/lucenenet/commit/6a81f860
>> >> > > > Tree:
>> >> http://git-wip-us.apache.org/repos/asf/lucenenet/tree/6a81f860
>> >> > > > Diff:
>> >> http://git-wip-us.apache.org/repos/asf/lucenenet/diff/6a81f860
>> >> > > >
>> >> > > > Branch: refs/heads/failingtests
>> >> > > > Commit: 6a81f860671ab98fb7cd595af317b3d8521acc21
>> >> > > > Parents: bdf2899
>> >> > > > Author: Laimonas Simutis <laimis@gmail.com>
>> >> > > > Authored: Sat May 30 14:49:35 2015 -0400
>> >> > > > Committer: Laimonas Simutis <laimis@gmail.com>
>> >> > > > Committed: Sat May 30 14:49:35 2015 -0400
>> >> > > >
>> >> > > >
>> >> ----------------------------------------------------------------------
>> >> > > >  src/Lucene.Net.TestFramework/Search/QueryUtils.cs | 4 ++--
>> >> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
>> >> > > >
>> >> ----------------------------------------------------------------------
>> >> > > >
>> >> > > >
>> >> > > >
>> >> > > >
>> >> > >
>> >> >
>> >>
>> http://git-wip-us.apache.org/repos/asf/lucenenet/blob/6a81f860/src/Lucene.Net.TestFramework/Search/QueryUtils.cs
>> >> > > >
>> >> ----------------------------------------------------------------------
>> >> > > > diff --git a/src/Lucene.Net.TestFramework/Search/QueryUtils.cs
>> >> > > > b/src/Lucene.Net.TestFramework/Search/QueryUtils.cs
>> >> > > > index 1156eee..6615d4c 100644
>> >> > > > --- a/src/Lucene.Net.TestFramework/Search/QueryUtils.cs
>> >> > > > +++ b/src/Lucene.Net.TestFramework/Search/QueryUtils.cs
>> >> > > > @@ -478,8 +478,8 @@ namespace Lucene.Net.Search
>> >> > > >                          Assert.IsTrue(scorer_.Advance(i)
!=
>> >> > > > DocIdSetIterator.NO_MORE_DOCS, "query collected " + doc +
" but
>> >> > skipTo("
>> >> > > +
>> >> > > > i + ") says no more docs!");
>> >> > > >                          Assert.AreEqual(doc, scorer_.DocID(),
>> >> "query
>> >> > > > collected " + doc + " but skipTo(" + i + ") got to " +
>> >> > scorer_.DocID());
>> >> > > >                          float skipToScore = scorer_.Score();
>> >> > > > -                        Assert.AreEqual(skipToScore,
>> >> scorer_.Score(),
>> >> > > > MaxDiff, "unstable skipTo(" + i + ") score!");
>> >> > > > -                        Assert.AreEqual(score, skipToScore,
>> >> MaxDiff,
>> >> > > > "query assigned doc " + doc + " a score of <" + score
+ "> but
>> >> skipTo("
>> >> > > + i
>> >> > > > + ") has <" + skipToScore + ">!");
>> >> > > > +                        Assert.IsTrue(Math.Abs(skipToScore
-
>> >> > > > scorer_.Score()) < MaxDiff, "unstable skipTo(" + i + ")
score!");
>> >> > > > +                        Assert.AreEqual(Math.Abs(score -
>> >> skipToScore)
>> >> > <
>> >> > > > MaxDiff, "query assigned doc " + doc + " a score of <"
+ score +
>> ">
>> >> but
>> >> > > > skipTo(" + i + ") has <" + skipToScore + ">!");
>> >> > > >
>> >> > > >                          // Hurry things along if they are
going
>> >> slow
>> >> > (eg
>> >> > > >                          // if you got SimpleText codec this
will
>> >> kick
>> >> > > in):
>> >> > > >
>> >> > > >
>> >> > >
>> >> >
>> >>
>> >
>> >
>>
>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message