INCF / csa

The Python implementation of the Connection-Set Algebra
GNU General Public License v3.0
13 stars 17 forks source link

Fixed FanIn/Out-Operator and SampleNRandomOperator #1

Closed pherbers closed 7 years ago

pherbers commented 7 years ago

This commit fixes the FanInOperator, FanOutOperator and SampleNOperator failing when generating connections from their respective iterators on 64-bit python builds. When generating the iterator, the numpy random function was seeded using a hash function. This hash function would generate a 64-bit integer on 64-bit python builds, while numpy.random.seed() only takes 32-bit integers. This caused the iterator generation to fail without error messages, as the function was being called from pycsa.cpp.

This is fixed in this commit by limiting the returned hash to a 32-bit integer using modulo.

mdjurfeldt commented 7 years ago

Dear Patrick,

Many thanks for finding this bug!

I'm a bit confused by your expression, though. Wouldn't it be cleaner to say:

numpy.random.seed (hash (seed) % (numpy.iinfo (numpy.int32).max + 1))

?

The abs is not needed since the modulo has the same sign as the divisor. Also, we use the full range.

Perhaps even better (more to the point; also more efficient, even though this doesn't matter much here) would be would be to use a bit operation, like an and:

numpy.random.seed (hash (seed) & numpy.iinfo (numpy.int32).max)

Please protest or modify your pull-request according to this and I'll merge!

pherbers commented 7 years ago

Hello Mikael,

You are right, in python the modulo operator only returns positive numbers, so the abs() is not needed at this point. I have also switched the numpy.int32 * 2 with numpy.uint32 since the seed function is actually using uint instead of int.

I left the modulo operator instead of the &-operator as it does not give any significant speed in python but is more readable this way.

Best, Patrick