TENNLab-UTK / fpga

FPGA neuromorphic elements, networks, processors, tooling, and software interfaces.
Mozilla Public License 2.0
1 stars 0 forks source link

RISP neuron model fire comparison is incorrectly evaluated #15

Closed BGull00 closed 2 weeks ago

BGull00 commented 1 month ago

In risp_neuron.sv, the following line determines whether or not a neuron fires: fire = sum >= (THRESHOLD + !THRESHOLD_INCLUSIVE);

In the case of THRESHOLD=1, THRESHOLD_INCLUSIVE=1, and sum=-1, this comparison incorrectly evaluates to fire=1. This comparison is evaluated correctly upon changing !THRESHOLD_INCLUSIVE to a literal 0. This comparison is also evaluated correctly upon changing !THRESHOLD_INCLUSIVE to ~THRESHOLD_INCLUSIVE.

keegandent commented 1 month ago

Weird, I'm going to see if I can find other places where there is arithmetic with !var expressions and check them too.

keegandent commented 4 weeks ago

So this wasn't actually an issue with !var math but instead a side effect of #14. In my PR, I did add support for the threshold_inclusive JSON parameter that wasn't there, as well as the non_negative_charge parameter. I will at some point implement fire_like_ravens.

BGull00 commented 3 weeks ago

I'm still having this same issue with my test XOR network upon further evaluation. Here is a screenshot demonstrating the issue. Notice that when the highlighted sum signal for neuron 2 = -1, fire is high despite the threshold being 1 (-1 < 1 so this should not be the case). Screenshot from 2024-08-16 17-44-35

keegandent commented 3 weeks ago

That's so bizarre; Can you tell me what number it says for POTENTIAL_ABS_MAX? If that logic is working I haven't the faintest idea what could be screwing with the right side of that sum comparison.

After that, could you please cherry-pick this commit and see if that fixes it? What is the sim value for THRESHOLD_ACTUAL?

keegandent commented 3 weeks ago

Something is really funky here because it won't fire sum = 0 but will when sum = -1

keegandent commented 3 weeks ago

It is occurs to me that asking the synthesis tool to compare two values is equivalent to asking it to subtract the right-hand side and compare to 0. So maybe I should make that explicit and adjust SUM_WIDTH, perhaps renaming sum to something like fire_integral? Just thinking out loud