arithmetic exception at nupic::Random::getUInt32(unsigned int)

[Thanh-Binh]

sometimes I had this exception

Program received signal SIGFPE, Arithmetic exception.
nupic::Random::getUInt32(unsigned int) (this=0xb240c8, max=0)
    at /home/binh/nupic/nupic.core/src/nupic/utils/Random.cpp:180
180	  UInt32 smax = Random::MAX32 - (Random::MAX32 % max);
(gdb) print max
$1 = 0
(gdb) 

in the function in the file Random.cpp:

UInt32 Random::getUInt32(const UInt32 max) {
  NTA_ASSERT(max > 0);
  UInt32 smax = Random::MAX32 - (Random::MAX32 % max);
  UInt32 sample;
  do {
    sample = impl_->getUInt32();
  } while (sample > smax);

  // NTA_WARN << "Random32(" << max << ") -> " << sample % max << " smax = " <<
  // smax;
  return sample % max;
}

Because here the value of max = 0, so that I believe that NTA_ASSERT(max > 0) does not work correctly, but why?
Do you have any idea?
thanks.

[rhyolight]

That is strange. Is this something you can write a little code to replicate? or better yet a unit test?

[lscheinkman]

NTA_ASSERT is only enabled on debug builds. Make sure to pass -DCMAKE_BUILD_TYPE=Debug when invoking cmake .

[Thanh-Binh]

But we have a problem in release mode, where the inputs lie is zero

[lscheinkman]

If you need that check to be compiled in release mode then you could replace the NTA_ASSERT macro with NTA_CHECK.

[breznak]

Yes, I believe the correct approach is NTA_CHECK, as modulo 0 does not make sense.
@rhyolight this shows it would be nice to run some CI in both Debug, Release mode.

[mrcslws]

We should stick with NTA_ASSERT. There's no reason to slow down the Release code with this check, especially since the function will already crash if max = 0. What's the benefit of making the program crash one line sooner? The purpose of this NTA_ASSERT is really just code readability.

[breznak]

@lscheinkman could you explain the motivation for the do..while loops? As removed in the above PR.
@Thanh-Binh you can use to community version, where this is fixed now.

[mrcslws]

The do...while is important for making sure all return values are equally likely. I just posted an explanation here: #1434

Regarding this issue, I don't think there's any fix needed. The NTA_ASSERT is serving its purpose.

[Thanh-Binh]

@lscheinkman @mrcslws : For debugging problem with getUInt32(), today I successfully compile both nupic_core and htmresearch-core in debug mode and release as mentioned in the README.

So I can compile and run my application in release mode.

BUT in the debug mode, I always have a linking error to debug library like:
``` undefined reference to `nupic::algorithms::temporal_memory::TemporalMemory::TemporalMemory(std::vector<unsigned int, std::allocator >, unsigned int, unsigned int, float, float, unsigned int, unsigned int, float, float, float, int, unsigned int, unsigned int, bool)'
collect2: error: ld returned 1 exit status

I really do not understand, why the constructor of TemporalMemory is disabled in debug mode?
By using
objdump -T libnupic_core.a
I found TemporalMemory.cpp.o is included in this library.

Do you have any idea?
Thanks