search

jeanluct / braidlab

Matlab package for analyzing data using braids
GNU General Public License v3.0
23 stars 9 forks source link

eqfuzzy not vectorized #114

Closed jeanluct closed 9 years ago

jeanluct commented 9 years ago

From @braid/private:

>> a = [1 1]; b = [2 2];
>> eqfuzzy(a,b,1e-5)
Operands to the || and && operators must be convertible to logical scalar values.

Error in eqfuzzy>nextafter (line 80)
if f == 1/2 && sign(x) ~= sign(d),

Error in eqfuzzy (line 43)
result(sel) = B(sel) <= nextafter(A(sel), D);

Not sure why this wasn't exposed before. I found it when trying devel/research/closure_test. (Change parfor to for for easier debugging. In fact the parfor is probably obsolete now.)

Maybe add a unit test for this.

jeanluct commented 9 years ago

I think I fixed it. Just a matter of changing && to &.

Doesn't eqfuzzy seem a little overwrought to you? Why doesn't it just do abs(a-b) < delta or something?

jeanluct commented 9 years ago

Ok, well in any case I will close this.

mbudisic commented 9 years ago

abs(a-b) < delta works if you know the scale of either a or b that you're comparing or if you have a physically-meaningful delta that you want to use. Since floating point error varies depending on the point on the real line, fixing delta across the board might be difficult in general purpose code. E.g. delta that works for two numbers around zero, the familiar 1e-16, will be too small for any meaningful tolerance of numbers around say 10,000 where you should use something like 1e-12 (as no two representable numbers will ever be less than 1e-12 apart). This is the number that MATLABs eps(a) returns.

The way to resolve this more precisely is testing whether a and b are within N representable numbers of each other. A good long explanation is given here: http://randomascii.wordpress.com/2012/02/25/comparing-floating-point-numbers-2012-edition/

I used it in C++ because STL has simple functions to return closest representable floats, instead of trying to judge what is a good eps to use, and then I included the same functionality in Matlab because I wanted to have consistent results between the two. If you feel it's overwrought, feel free to cut it out and replace with something simpler (I am not vested in keeping this in).

jeanluct commented 9 years ago

I see, good point: my naive solution wouldn't be very good.

I guess it's fine if C++ has a fast way to do this. It just looks very kludgy when implemented in Matlab, esp. since this is the kind of function that you might justifiably expect to evaluate as fast as ==.

So it's fine: users who use the non-MEX braidlab are in for a slow time anyways.

mbudisic commented 9 years ago

Yeah, it is very cludgy in Matlab, that's why I didn't have it originally... but then we had a bug of some sort (I think this is what Michael or Margaux had a problem with), and I really needed to make sure that the equality testing was not the culprit...

jeanluct commented 9 years ago

Right, it's (fairly) important that the Matlab and C++ version give the same answer, within reason.

On Wed, Jan 7, 2015 at 9:07 AM, Marko Budisic notifications@github.com wrote:

Yeah, it is very cludgy in Matlab, that's why I didn't have it originally... but then we had a bug of some sort (I think this is what Michael or Margaux had a problem with), and I really needed to make sure that the equality testing was not the culprit...

— Reply to this email directly or view it on GitHub https://github.com/jeanluct/braidlab/issues/114#issuecomment-69034004.