search

jeanluct / braidlab

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

areEqual does not conform to how we typically write M/MEX #92

Closed mbudisic closed 9 years ago

mbudisic commented 9 years ago

areEqual compares two float numbers up to D nearest float-representable numbers. It was originally written as a C++ function but it does not conform to how we usually write them.

First of all, it is not a _helper.cpp as this was a core functionality to begin with. I have re-written it in Matlab this morning so now areEqual.m does run fine without MEX compiled version (although potentially slower).

To do:

  1. Move areEqual to braidlab.util as it is not braid specific and it might be something we'd like to use outside just braid class. This means that Makefile and invocations should be changed to reflect this.
  2. Write the C++ code as a _helper function?
  3. Write unit test that compares areEqual.m with areEqual.cpp. This might be tricky to write as it is really about how "tightly" two precision-testing functions are.

I wouldn't call this a high-priority issue, but we should get to it eventually.

jeanluct commented 9 years ago

But at this point no other class needs it, right? I'd rather keep it out of +util until we need it. It also doesn't follow the naming convention: it has a capital letter, so if you move it to +util better rename it areequal. But even then I don't love the name, since it doesn't really say what it does. How about fuzzyeq, since eq is the name of the equal operator in Matlab?

mbudisic commented 9 years ago

All valid points...

jeanluct commented 9 years ago

Looks good. Closing (reopen if you still need to do things).