search

TENNLab-UTK / fpga

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

Processor apply_spike method should translate unit range to discrete charge integers #10

Closed keegandent closed 2 weeks ago

keegandent commented 1 month ago

Problem

The current implementation of the apply_spikes method presumes the charge value being supplied is relative to the threshold specified in the network JSON and the scale of max_weight. Per @jimplank, @BGull00, and a brief search into the TENNLab Framework documentation, this is not the case.

Proposed Solution

Look to see what scaling is done for discrete networks in the RISP & RAVENS code and try to follow that as closely as possible for the FPGA Processor. If there is a difference between the two, bring it to the attention of Framework maintainers.

Hurdles

Because of the inherently unbalance nature of signed integers (e.g., 4-bit is $[-8, 7]$) it seems that if we use the full scale of values but have $f(0.0) = 0$, we will end up with asymmetric resolution in mapping. My guess is this doesn't have significant impact on network performance or behavior but depending on RISP & RAVENS implementation, it may be worth re-evaluating, and it at the very least should be documented here and in the parent Framework. The scaling in the Framework is done purely based on maximum values, so I will have the processor issue a warning if the minimum parameter is not the same magnitude.

Expected API Impacts

This will create a breaking change in fpga.Processor.apply_spikes

keegandent commented 1 month ago

@jimplank, @BGull00,

I looked into the Implementation of spike scaling for both RISP and RAVENS, and I have a concern.

The scaling for both RISP and RAVENS appears to be based around the max_threshold parameter. In RAVENS, this is inferred from the default params/ravens.json which specifies the following.

"min_weight": -8,
"max_weight": 7,
"..." : "...",
"min_threshold": 0,
"max_threshold": 15,
"..." : "...",
"spike_value_factor": 16

In RISP, this is more overt:

double Processor::get_input_spike_factor() const
{
  if (!discrete) return max_threshold;
  if (!threshold_inclusive) return max_threshold+1;
  return max_threshold;
}

This choice creates complications with hardware design. Because the threshold for an individual neuron is something completely internal to that neuron, we don't have to worry about sizing for it. It could be 1 or 300 or 2047 or 50 million for all anyone cares; the design will allocate the appropriate number of bits for "potential" based on this code in risp_neuron.sv

localparam POTENTIAL_ABS_MAX = ((THRESHOLD < 0) ? -THRESHOLD : THRESHOLD) + !THRESHOLD_INCLUSIVE;
localparam POTENTIAL_WIDTH = $clog2(POTENTIAL_ABS_MAX) + 1; // sign bit

What we do care about is weight. Each synapse weight dictates exactly what charge a neuron can expect from a given synapse, so the bounds on those weights dictate that "charge width" across the entire network. There are likely areas this gets optimized out by the toolchain, but this must be a weight-driven parameter because the input interfaces for neurons are defined by weight, not threshold.

TL;DR: I think input_scaling_value, spike_value_factor, etc. in the Framework need to be based on maximum synapse weight, not maximum neuron threshold. Otherwise it could significantly complicate design, as neurons would each need a separate port of a different width which is only active if they are input neurons. Aside from just design aesthetics, it doesn't make sense, at least to me, for input spikes to be fundamentally different than ones from synapses and should use the same scale.

jimplank commented 1 month ago

Hi Keegan — I believe the rationale behind the decision is that we'd like an input value of 1 from the framework to force a spike on the neuron. That said, I don't really care about the rationale — if we set the input_spike_factor to the maximum synapse weight, then it's up to you when you design the neural network to have your input thresholds be low enough. It may hamper / limit EONS, but that's a different matter.

I think the only significant issue is compatibility with RAVENS. RAVENs handles inputs as follows:

Charge Injection: There are two ways to apply external input to a RAVENS network. The first is to simply cause one of the neuron’s pre-synapses to spike. The second way is to allocate a number of synapse “ports” for charge injection. This number is a Hardware Constant. Then, it is a Neuron Setting to enable the ports. If a neuron’s ports are enabled, then input values may be delivered to the neuron using the ports as a signed binary number.

Charlie, do we ever use the charge injection feature when we're using RAVENs, or do we simply cause synapses to spike?

Here's a question — Keegan, are you implementing all of RISP, or just a subset? If it's a subset, then we can simply make it well-defined, and I'll add a spike_value_factor parameter to it, and your problems are solved. If it's not, then we should consider having it default to the max synapse weight, and perhaps disabling charge injection in RAVENs, which will only simplify it. I'd like to retain RISP/RAVENs settings that make them compatible.

Jim

On Aug 1, 2024, at 12:25 PM, Keegan Dent @.***> wrote:

@jimplankhttps://github.com/jimplank, @BGull00https://github.com/BGull00,

I looked into the Implementation of spike scaling for both RISP and RAVENS, and I have a concern.

The scaling for both RISP and RAVENS appears to be based around the max_threshold parameter. In RAVENS, this is inferred from the default params/ravens.json which specifies the following.

"min_weight": -8, "max_weight": 7, "..." : "...", "min_threshold": 0, "max_threshold": 15, "..." : "...", "spike_value_factor": 16

In RISP, this is more overt:

double Processor::get_input_spike_factor() const { if (!discrete) return max_threshold; if (!threshold_inclusive) return max_threshold+1; return max_threshold; }

This choice creates complications with hardware design. Because the threshold for an individual neuron is something completely internal to that neuron, we don't have to worry about sizing for it. It could be 1 or 300 or 2047 or 50 million for all anyone cares; the design will allocate the appropriate number of bits for "potential" based on this code in risp_neuron.sv

localparam POTENTIAL_ABS_MAX = ((THRESHOLD < 0) ? -THRESHOLD : THRESHOLD) + !THRESHOLD_INCLUSIVE; localparam POTENTIAL_WIDTH = $clog2(POTENTIAL_ABS_MAX) + 1; // sign bit

What we do care about is weight. Each synapse weight dictates exactly what charge a neuron can expect from a given synapse, so the bounds on those weights dictate that "charge width" across the entire network. There are likely areas this gets optimized out by the toolchain, but this must be a weight-driven parameter because the input interfaces for neurons are defined by weight, not threshold.

TL;DR: I think input_scaling_value, spike_value_factor, etc. in the Framework need to be based on maximum synapse weight, not maximum neuron threshold. Otherwise it could significantly complicate design, as neurons would each need a separate port of a different width which is only active if they are input neurons. Aside from just design aesthetics, it doesn't make sense, at least to me, for input spikes to be fundamentally different than ones from synapses and should use the same scale.

— Reply to this email directly, view it on GitHubhttps://github.com/TENNLab-UTK/fpga/issues/10#issuecomment-2263471958, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AGEAIRA5TZLBFHC2MXK7SD3ZPJOOBAVCNFSM6AAAAABLY7NY6SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENRTGQ3TCOJVHA. You are receiving this because you were mentioned.Message ID: @.***>

jimplank commented 1 month ago

Hi all,

Typically, when we train, we set spike_value_factor = max_threshold + 1 in order to force the input neuron to fire when an input spike/value is applied to it. This does not always need to be the case though, as you could make a case for having an input neuron that does not fire immediately as soon as it gets a spike and instead must accumulate a certain amount of spikes to fire (adding leak to this equation further complicates it, but I digress).

The reason charge_injection exists on the hardware side is so that we can basically do the Val encoder. It was my understanding that without it, we could not directly apply a value to the neuron. Given that we do use the Val encoder when training across different apps, I do think it is relevant. Though, I will note, we have not verified nor even tested the parameter on hardware. I have support for it in my scan chain generator, but we have not gotten around to testing that parameter given that all of our work on hardware so far has only leveraged the simple spike encoder.

I’m not sure if that totally answers the question, but there is a cause for it for the sake of the Val encoder (which we could try to start testing).

I think basing spike_value_factor on synapse weight could be just fine. After all, it’s really just a handle on forcing the input neuron to fire.

Charles P. Rizzo Research Assistant Professor Department of Electrical Engineering and Computer Science University of Tennessee Email: @.**@.> On Aug 4, 2024 at 4:39 PM -0400, Plank, James S @.***>, wrote: Hi Keegan — I believe the rationale behind the decision is that we'd like an input value of 1 from the framework to force a spike on the neuron. That said, I don't really care about the rationale — if we set the input_spike_factor to the maximum synapse weight, then it's up to you when you design the neural network to have your input thresholds be low enough. It may hamper / limit EONS, but that's a different matter.

I think the only significant issue is compatibility with RAVENS. RAVENs handles inputs as follows:

Charge Injection: There are two ways to apply external input to a RAVENS network. The first is to simply cause one of the neuron’s pre-synapses to spike. The second way is to allocate a number of synapse “ports” for charge injection. This number is a Hardware Constant. Then, it is a Neuron Setting to enable the ports. If a neuron’s ports are enabled, then input values may be delivered to the neuron using the ports as a signed binary number.

Charlie, do we ever use the charge injection feature when we're using RAVENs, or do we simply cause synapses to spike?

Here's a question — Keegan, are you implementing all of RISP, or just a subset? If it's a subset, then we can simply make it well-defined, and I'll add a spike_value_factor parameter to it, and your problems are solved. If it's not, then we should consider having it default to the max synapse weight, and perhaps disabling charge injection in RAVENs, which will only simplify it. I'd like to retain RISP/RAVENs settings that make them compatible.

Jim

On Aug 1, 2024, at 12:25 PM, Keegan Dent @.***> wrote:

@jimplankhttps://github.com/jimplank, @BGull00https://github.com/BGull00,

I looked into the Implementation of spike scaling for both RISP and RAVENS, and I have a concern.

The scaling for both RISP and RAVENS appears to be based around the max_threshold parameter. In RAVENS, this is inferred from the default params/ravens.json which specifies the following.

"min_weight": -8, "max_weight": 7, "..." : "...", "min_threshold": 0, "max_threshold": 15, "..." : "...", "spike_value_factor": 16

In RISP, this is more overt:

double Processor::get_input_spike_factor() const { if (!discrete) return max_threshold; if (!threshold_inclusive) return max_threshold+1; return max_threshold; }

This choice creates complications with hardware design. Because the threshold for an individual neuron is something completely internal to that neuron, we don't have to worry about sizing for it. It could be 1 or 300 or 2047 or 50 million for all anyone cares; the design will allocate the appropriate number of bits for "potential" based on this code in risp_neuron.sv

localparam POTENTIAL_ABS_MAX = ((THRESHOLD < 0) ? -THRESHOLD : THRESHOLD) + !THRESHOLD_INCLUSIVE; localparam POTENTIAL_WIDTH = $clog2(POTENTIAL_ABS_MAX) + 1; // sign bit

What we do care about is weight. Each synapse weight dictates exactly what charge a neuron can expect from a given synapse, so the bounds on those weights dictate that "charge width" across the entire network. There are likely areas this gets optimized out by the toolchain, but this must be a weight-driven parameter because the input interfaces for neurons are defined by weight, not threshold.

TL;DR: I think input_scaling_value, spike_value_factor, etc. in the Framework need to be based on maximum synapse weight, not maximum neuron threshold. Otherwise it could significantly complicate design, as neurons would each need a separate port of a different width which is only active if they are input neurons. Aside from just design aesthetics, it doesn't make sense, at least to me, for input spikes to be fundamentally different than ones from synapses and should use the same scale.

— Reply to this email directly, view it on GitHubhttps://github.com/TENNLab-UTK/fpga/issues/10#issuecomment-2263471958, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AGEAIRA5TZLBFHC2MXK7SD3ZPJOOBAVCNFSM6AAAAABLY7NY6SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENRTGQ3TCOJVHA. You are receiving this because you were mentioned.Message ID: @.***>

jimplank commented 1 month ago

Thanks, Charlie — Keegan, are you implementing all of the RISP parameters, or only a subset? I'm assuming you aren't doing floating point, but I'm not sure. Please let me know — thanks! JIm

On Aug 4, 2024, at 4:59 PM, Rizzo, Charles Peter @.***> wrote:

Hi all,

Typically, when we train, we set spike_value_factor = max_threshold + 1 in order to force the input neuron to fire when an input spike/value is applied to it. This does not always need to be the case though, as you could make a case for having an input neuron that does not fire immediately as soon as it gets a spike and instead must accumulate a certain amount of spikes to fire (adding leak to this equation further complicates it, but I digress).

The reason charge_injection exists on the hardware side is so that we can basically do the Val encoder. It was my understanding that without it, we could not directly apply a value to the neuron. Given that we do use the Val encoder when training across different apps, I do think it is relevant. Though, I will note, we have not verified nor even tested the parameter on hardware. I have support for it in my scan chain generator, but we have not gotten around to testing that parameter given that all of our work on hardware so far has only leveraged the simple spike encoder.

I’m not sure if that totally answers the question, but there is a cause for it for the sake of the Val encoder (which we could try to start testing).

I think basing spike_value_factor on synapse weight could be just fine. After all, it’s really just a handle on forcing the input neuron to fire.

Charles P. Rizzo Research Assistant Professor Department of Electrical Engineering and Computer Science University of Tennessee Email: @.**@./> On Aug 4, 2024 at 4:39 PM -0400, Plank, James S @.***>, wrote: Hi Keegan — I believe the rationale behind the decision is that we'd like an input value of 1 from the framework to force a spike on the neuron. That said, I don't really care about the rationale — if we set the input_spike_factor to the maximum synapse weight, then it's up to you when you design the neural network to have your input thresholds be low enough. It may hamper / limit EONS, but that's a different matter.

I think the only significant issue is compatibility with RAVENS. RAVENs handles inputs as follows:

Charge Injection: There are two ways to apply external input to a RAVENS network. The first is to simply cause one of the neuron’s pre-synapses to spike. The second way is to allocate a number of synapse “ports” for charge injection. This number is a Hardware Constant. Then, it is a Neuron Setting to enable the ports. If a neuron’s ports are enabled, then input values may be delivered to the neuron using the ports as a signed binary number.

Charlie, do we ever use the charge injection feature when we're using RAVENs, or do we simply cause synapses to spike?

Here's a question — Keegan, are you implementing all of RISP, or just a subset? If it's a subset, then we can simply make it well-defined, and I'll add a spike_value_factor parameter to it, and your problems are solved. If it's not, then we should consider having it default to the max synapse weight, and perhaps disabling charge injection in RAVENs, which will only simplify it. I'd like to retain RISP/RAVENs settings that make them compatible.

Jim

On Aug 1, 2024, at 12:25 PM, Keegan Dent @.***> wrote:

@jimplankhttps://github.com/jimplank, @BGull00https://github.com/BGull00,

I looked into the Implementation of spike scaling for both RISP and RAVENS, and I have a concern.

The scaling for both RISP and RAVENS appears to be based around the max_threshold parameter. In RAVENS, this is inferred from the default params/ravens.json which specifies the following.

"min_weight": -8, "max_weight": 7, "..." : "...", "min_threshold": 0, "max_threshold": 15, "..." : "...", "spike_value_factor": 16

In RISP, this is more overt:

double Processor::get_input_spike_factor() const { if (!discrete) return max_threshold; if (!threshold_inclusive) return max_threshold+1; return max_threshold; }

This choice creates complications with hardware design. Because the threshold for an individual neuron is something completely internal to that neuron, we don't have to worry about sizing for it. It could be 1 or 300 or 2047 or 50 million for all anyone cares; the design will allocate the appropriate number of bits for "potential" based on this code in risp_neuron.sv

localparam POTENTIAL_ABS_MAX = ((THRESHOLD < 0) ? -THRESHOLD : THRESHOLD) + !THRESHOLD_INCLUSIVE; localparam POTENTIAL_WIDTH = $clog2(POTENTIAL_ABS_MAX) + 1; // sign bit

What we do care about is weight. Each synapse weight dictates exactly what charge a neuron can expect from a given synapse, so the bounds on those weights dictate that "charge width" across the entire network. There are likely areas this gets optimized out by the toolchain, but this must be a weight-driven parameter because the input interfaces for neurons are defined by weight, not threshold.

TL;DR: I think input_scaling_value, spike_value_factor, etc. in the Framework need to be based on maximum synapse weight, not maximum neuron threshold. Otherwise it could significantly complicate design, as neurons would each need a separate port of a different width which is only active if they are input neurons. Aside from just design aesthetics, it doesn't make sense, at least to me, for input spikes to be fundamentally different than ones from synapses and should use the same scale.

— Reply to this email directly, view it on GitHubhttps://github.com/TENNLab-UTK/fpga/issues/10#issuecomment-2263471958, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AGEAIRA5TZLBFHC2MXK7SD3ZPJOOBAVCNFSM6AAAAABLY7NY6SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENRTGQ3TCOJVHA. You are receiving this because you were mentioned.Message ID: @.***>

keegandent commented 1 month ago

@jimplank, Struggled to understand a bit of the above as it seems Charlie's response came through your handle, but I think I get it now.

I looked into floating point on FPGAs and it's almost universally avoided. The implementations suffer primarily from poor portability between platforms, as each vendor implements with different deviations from the IEEE 754 standard, varying levels of performance, and different degrees of coupling into higher-order modules such as DSP slices. Most hardware designers suggest a thorough understanding of the numerics involved and a fixed-point software model as an intermediate comparison for the floating point model and hardware implementation (what y'all have already accomplished with discrete=True).

But minus discrete=False processors, my aim is for software sim to be identical to FPGA implementation and for every other aspect of the parameter space to be implemented... eventually.

I don't really understand the difference between "charge injection" and the pre-synapse spiking, but maybe we can have a sidebar after Monday's meeting. It sounds like my current packet implementation is the charge injection system?

If we want to ensure that with this change input neurons still always spike, we could have the framework enforce a rule that threshold can never exceed max_weight + !threshold_inclusive, but maybe that would create too many issues for existing networks or the tapeout processors?

keegandent commented 1 month ago

Also @jimplank, it seems there may already be a field for the spike factor on RISP, but it has a different name, (input_scaling_value) from the RAVENS one? Though this code also appears in RAVENS. despite having a separate key for spike_value_factor.

json Processor::get_processor_properties() const {

  json j = json::object();

  j["input_scaling_value"] = get_input_spike_factor();
  j["binary_input"] = false;
  j["spike_raster_info"] = true;
  j["plasticity"] = "none";
  j["run_time_inclusive"] = run_time_inclusive;
  j["integration_delay"] = false;
  j["threshold_inclusive"] = threshold_inclusive;

  return j;
}
jimplank commented 1 month ago

Hi Keegan — so, get_processor_properties() is a method that we introduced early on so that other pieces of software could get some information that should be common to all neuroprocessors. I don't think it's used — it's documented in https://bitbucket.org/neuromorphic-utk/framework/src/master/markdown/framework_processor.md (just search for get_processor).

Responding to your previous message about charge injection — sure — let's talk today after the bandit meeting (is there a bandit meeting? Amanda's email on 8/2 said she'd be on PTO today). Regardless, I'm free pretty much all day, so regardless of Bandit, I'd be happy to meet, and I'm sure we can get Charlie and Bryson to meet as well — Jim

On Aug 5, 2024, at 10:16 AM, Keegan Dent @.***> wrote:

Also @jimplankhttps://github.com/jimplank, it seems there may already be a field for the spike factor on RISP, but it has a different name, (input_scaling_value) from the RAVENS one?

json Processor::get_processor_properties() const {

json j = json::object();

j["input_scaling_value"] = get_input_spike_factor(); j["binary_input"] = false; j["spike_raster_info"] = true; j["plasticity"] = "none"; j["run_time_inclusive"] = run_time_inclusive; j["integration_delay"] = false; j["threshold_inclusive"] = threshold_inclusive;

return j; }

— Reply to this email directly, view it on GitHubhttps://github.com/TENNLab-UTK/fpga/issues/10#issuecomment-2269188646, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AGEAIRAEP4NSN4Z73KFYM4TZP6CLJAVCNFSM6AAAAABLY7NY6SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENRZGE4DQNRUGY. You are receiving this because you were mentioned.Message ID: @.***>

keegandent commented 1 month ago

Outcome of our discussion today regarding this:

For FPGA networks, intermediate (and packet) representation of charge will continue to be based on max_weight. The behavior will be defined in this manner.

Defaults

Warnings

Errors

It has occurred to me that discrete && spike_value_factor == 1 could be the trigger for binary_mode for #8 because it by definition only allows 0 or 1.

Let me know if I'm missing anything here.

keegandent commented 4 weeks ago

@jimplank Do you want to add the Framework RISP code for spike_value_factor, or can I take a stab at it and open a pull request in BitBucket?

jimplank commented 4 weeks ago

If you don't mind, I'll do it. I'm not saying you can't, but I like to be the responsible person in case something goes wrong. I'll do it this morning.

On Aug 14, 2024, at 12:38 AM, Keegan Dent @.***> wrote:

@jimplankhttps://github.com/jimplank Do you want to add the Framework RISP code for spike_value_factor, or can I take a stab at it and open a pull request in BitBucket?

— Reply to this email directly, view it on GitHubhttps://github.com/TENNLab-UTK/fpga/issues/10#issuecomment-2287839623, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AGEAIRBI3GLC56GYM3KLXK3ZRLNMTAVCNFSM6AAAAABLY7NY6SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEOBXHAZTSNRSGM. You are receiving this because you were mentioned.Message ID: @.***>

jimplank commented 4 weeks ago

I've just pushed the change to the framework — there is now a "input_scaling_value" parameter. It defaults to max_threshold or max_threshold+1, but you can change it.

keegandent commented 4 weeks ago

I've just pushed the change to the framework — there is now a "input_scaling_value" parameter. It defaults to max_threshold or max_threshold+1, but you can change it.

Thanks! But are we not changing to a default scaling of max_weight instead of max_threshold as discussed above? It will then fall on the user to avoid input neuron thresholds above the scaling factor (but a Framework warning would help too). I just want apply_spike(id, t, 1.0) to have identical meaning for sim and FPGA.

Also, I thought it would be called spike_value_factor so it has the same parameter as RAVENS, but I don't mind either way.

jimplank commented 4 weeks ago

To be honest with you, I don't really care about the name. It's a little frustrating that the name bifurcation came about with RAVENS. I'm thinking what we should do is use spike_value_factor, and then change get_processor_properties() to return "spike_value_factor" instead of "input_scaling_value." If I'm not mistaken, no one currently uses the output to get_processor_properties(), and we're only supporting 5 neuroprocessors (risp, ravens, mravens, izhikevich, caspian), so that change won't be hard (just getting the ORNL folks to change caspian).

I'm cc-ing Katie — thoughts?

I'm less sold on changing it as a default, because then any network created with risp_for_ravens.json will break (unless we do something weird, like default fire_like_ravens to the threshold, and anything else to the weight).

But open to discussion.

Jim

On Aug 14, 2024, at 11:09 AM, Keegan Dent @.***> wrote:

I've just pushed the change to the framework — there is now a "input_scaling_value" parameter. It defaults to max_threshold or max_threshold+1, but you can change it.

Thanks! But are we not changing to a default scaling of max_weight instead of max_threshold as discussed above? It will then fall on the user to avoid input neurons thresholds above the scaling factor (but a Framework warning would help too). I just want apply_spike(id, t, 1.0) to have identical meaning for sim and FPGA.

Also, I thought it would be called spike_value_factor so it has the same parameter as RAVENS, but I don't mind either way.

— Reply to this email directly, view it on GitHubhttps://github.com/TENNLab-UTK/fpga/issues/10#issuecomment-2289074103, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AGEAIRBEJNLUNPN2J4AI5TTZRNXK7AVCNFSM6AAAAABLY7NY6SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEOBZGA3TIMJQGM. You are receiving this because you were mentioned.Message ID: @.***>

keegandent commented 4 weeks ago

I'm less sold on changing it as a default, because then any network created with risp_for_ravens.json will break (unless we do something weird, like default fire_like_ravens to the threshold, and anything else to the weight).

I understand the hesitation in that case. How about this as a sort of compromise?

FPGA RISP will use the max of input_scaling_value/spike_value_factor and max_weight to determine charge width throughout the network, and the default for the scaling will remain max_threshold + !threshold_inclusive. This will potentially create wasted resources, so FPGA RISP will adopt the following behaviors and document them, perhaps in a USING.md.

Warnings

  1. node.isInput() && (node.values[thresh_idx] + !threshold_inclusive) > input_scaling_value_or_spike_value_factor: "An input neuron with an index of {} (input index {}) has a spiking threshold which exceeds the scaling factor {}."
  2. clog2(input_scaling_value_or_spike_value_factor - 1) >= clog2(max_weight - 1): "A scaling factor of {} will mandate a network-wide charge width greater than what is needed by the maximum synapse weight of {}, potentially wasting hardware resources."

Errors

  1. binary_mode && node.isInput() && (node.values[thresh_idx] + !threshold_inclusive) > ((1 << (charge_width - 1)) - 1)
  2. input_scaling_value_or_spike_value_factor < 1.0: "The scaling factor must be greater than or equal to 1"

Warning 2 could be elevated into an error if we want to enforce lean hardware usage, but I see this as a more forgiving nudge to the user. Warning 1 should possibly also be implemented in the Framework if users should be aware of thresholds that will cause apply_spike(id, t, 1.0) to not induce a fire.

jimplank commented 4 weeks ago

A couple of things:

  1. Does anyone besides me use risp_for_ravens.json? If not, then let's change this to what we want, and I can fix any networks that matter with shell scripts.
  2. I've never seen "%1 != 0" before.
  3. Is "Binary_Mode" defined somewhere?

My plan is to post on slack to see if anyone uses risp_for_ravens.json and if not, I'll change the default behavior of RISP. I'll also use spike_value_factor, and change that in get_properties() so that input_scaling_value is gone.

Anything else I should do with RISP while we're thinking about it?

On Aug 14, 2024, at 1:36 PM, Keegan Dent @.***> wrote:

I'm less sold on changing it as a default, because then any network created with risp_for_ravens.json will break (unless we do something weird, like default fire_like_ravens to the threshold, and anything else to the weight).

I understand the hesitation in that case. How about this as a sort of compromise?

FPGA RISP will use the max of input_scaling_value/spike_value_factor and max_weight to determine charge width throughout the network, and the default for the scaling will remain max_threshold + !threshold_inclusive. This will potentially create wasted resources, so FPGA RISP will adopt the following behaviors and document them, perhaps in a USING.md.

Warnings

  1. node.isInput() && node.values[thresh_idx] + !threshold_inclusive) > input_scaling_value_or_spike_value_factor: "An input neuron with an index of {} (input index {}) has a spiking threshold which exceeds the scaling factor {}."
  2. clog2(input_scaling_value_or_spike_value_factor) >= clog2(max_weight): "A scaling factor of {} will mandate a network-wide charge width greater than what is needed by the maximum synapse weight of {}, potentially wasting hardware resources."

Errors

  1. In binary_mode, Warning 1 becomes an error.
  2. input_scaling_value_or_spike_value_factor < 1 || input_scaling_value_or_spike_value_factor % 1 != 0: "The scaling factor must be an integer greater than or equal to 1"

Warning 2 could be elevated into an error if we want to enforce lean hardware usage, but I see this as a more forgiving nudge to the user. Warning 1 should possibly also be implemented in the Framework if users should be aware of thresholds that will cause apply_spike(id, t, 1.0) to not induce a fire. Likewise, the Framework should also implement Error 2 for discrete networks; we don't want non-integer scaling factors in that case.

— Reply to this email directly, view it on GitHubhttps://github.com/TENNLab-UTK/fpga/issues/10#issuecomment-2289421900, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AGEAIRBE4HAKIORFFBZBCV3ZROITDAVCNFSM6AAAAABLY7NY6SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEOBZGQZDCOJQGA. You are receiving this because you were mentioned.Message ID: @.***>

BGull00 commented 4 weeks ago

I’ve used risp_for_ravens.json before, but I don’t actively need it for any networks. Another RISP change we may want is a minimum negative potential value as discussed in Issue #17 instead of (or in addition to, depending on how many nets we’re ok breaking) non_negative_charge.

keegandent commented 4 weeks ago

A couple of things: 1. Does anyone besides me use risp_for_ravens.json? If not, then let's change this to what we want, and I can fix any networks that matter with shell scripts. 2. I've never seen "%1 != 0" before. 3. Is "Binary_Mode" defined somewhere? My plan is to post on slack to see if anyone uses risp_for_ravens.json and if not, I'll change the default behavior of RISP. I'll also use spike_value_factor, and change that in get_properties() so that input_scaling_value is gone. Anything else I should do with RISP while we're thinking about it?

  1. I'm assuming this is mostly @tennlab and not me, so I'll pass on this one. I would like to default to max_weight, but supporting max_threshold wouldn't be overly burdensome.
  2. I realized we actually may need to support non-integer scaling on discrete networks and have edited my previous comment on github, so please ignore that.
  3. binary_mode will be an FPGA source option as part of #8. We could have it inferred by discrete && spike_value_factor == 1 and then there would be no need to throw an error because input spikes could be automatically scaled by $2^{\text{charge\_width}-1}-1$. If that's too convoluted, we could leave it as a separate optional flag or derive from the encoder, app agent, etc. Whatever you all prefer.

To be clear, I don't care if it's called spike_value_factor or input_scaling_value. I marginally prefer the names being the same regardless of which ones change.

I’ve used risp_for_ravens.json before, but I don’t actively need it for any networks. Another RISP change we may want is a minimum negative potential value as discussed in Issue #17 instead of (or in addition to, depending on how many nets we’re ok breaking) non_negative_charge.

Seconded. I say support both params, but warn the user non_negative_charge=true is deprecated in favor of min_potential=0 and excluding the min_potential parameter will be taken to imply it is $-\infty$. JSON does not support NaN/Infinity float values despite being valid IEEE 754, so a user specifying non_negative_charge=false would be best served by leaving both parameters absent. In this way, I can just make it a required parameter for FPGA processors.

jimplank commented 3 weeks ago

I have sent the following message to our slack software channel. Please let me know if you see anything wrong with this plan for changing RISP in the framework:

This is for anyone who uses RISP: Because of the FPGA work, we are strongly considering making the following changes to RISP:

    1. Adding an explicit spike_value_factor for scaling input spikes, like there is with RAVENs. Currently, RISP scales by the maximum threshold (+1 if threshold_inclusive is false ). This parameter will be optional; however see #2 below.
    1. Changing the default behavior so that spike_value_factor defaults to max_weight (+1 if threshold_inclusive is false). The reasoning behind this is that it will make the FPGA implementation of the neuron more efficient, thereby allowing us greater neuron/synapse density. My concern with this change is that it may affect networks trained before the change. Fortunately, the defaults don't matter, and any network trained where max_weight equals max_threshold will be unaffected. Networks where max_weight exceeds max_threshold will be affected, and will need a spike_value_factor parameter added so that they work equivalently to before the change. I suspect very few people have networks like this, but I know I do. If you use the parameter file in risp_from_ravens, then you do too.
    1. Replacing non_negative_charge with min_potential, and making it required. Again, the reason for this is FPGA efficiency -- currently RISP (and its simulator), when non_negative_charge is false, places no bounds on the value of neuron potentials when they are negative. If they keep getting negatively charged spike from their synapses, then they can accumulate unbounded negative charge, and become impossible to spike. I don't think this is a massive change -- my guess is that if you set it to, say, -16, your networks will all work as before (and this is a situation where risp_from_ravens is unaffected).

My plan is to implement these changes, and to do the following:

@Jamin Hannahttps://neuromorphic-utk.slack.com/team/U05Q68E0NBA, I'm going to take a look at weights today and figure out the implications.Let me reiterate -- if you use the defaults with RISP, you will be unaffected. If you use risp_from_ravens, or any network where max_threshold does not equal max_weight, then you will be affected.Regardless, my plan is to work on this change starting monday, with the thought of having it merged into the main branch by the end of the week. So, if this is a problem for you, please let me know ASAP.

keegandent commented 3 weeks ago

@jimplank

  1. the default scale factor should not be max_weight + 1 when threshold_inclusive == false. It should still just be max_weight in this manner. So threshold-exclusive networks where max_weight == max_threshold will still need to manually set their spike factor to max_threshold + 1.
  2. The minimum potential change isn't particularly about being efficient, though a value with less magnitude will use fewer registers. It's really just bounding the Framework RISP sim in a way that allows the FPGA implementation to replicate it correctly. Otherwise whatever lower bound we chose would eventually be too conservative and users would (rightly) be confused or upset by disparate results.

On that note, I think it might be a good idea for me to create a random spike test that compares a user network on FPGA RISP and Framework RISP to verify they behave identically, basically comparing spike rasters again, just with arbitrary networks and spikes.

jimplank commented 3 weeks ago

2: Check.

3: My opinion on this is that we shouldn't allow it to be -infinity. We should bound it and call it a day. Perhaps the default should be either -max_threshold or maybe -2*max_threshold. Like the threshold, the value can go below this during integration — it's only clamped once integration is done.

My reasoning for not allowing infinity is that I don't believe that it's a useful case, except maybe for some hand-tooled networks with specific functionality. I also think that the more you can remove decision-making from users, the better off they will be. Just my two cents — I'm open to being talked out of it.

100% on board with testing networks. The processor tool in the framework is made for this.

On Aug 15, 2024, at 10:21 AM, Keegan Dent @.***> wrote:

@jimplankhttps://github.com/jimplank

  1. the default scale factor should not be max_weight + 1 when threshold_inclusive == false. It should still just be max_weight in this manner. So threshold-exclusive networks where max_weight == max_threshold will still need to manually set their spike factor to max_threshold + 1.
  2. The minimum potential change isn't particularly about being efficient, though a value with less magnitude will use fewer registers. It's really just bounding the Framework RISP sim in a way that allows the FPGA implementation to replicate it correctly. Otherwise whatever lower bound we chose would eventually be too conservative and users would (rightly) be confused or upset by disparate results.

On that note, I think it might be a good idea for me to create a random spike test that compares a user network on FPGA RISP and Framework RISP to verify they behave identically, basically comparing spike rasters again, just with arbitrary networks and spikes.

— Reply to this email directly, view it on GitHubhttps://github.com/TENNLab-UTK/fpga/issues/10#issuecomment-2291360613, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AGEAIRHDGXBWEE5LDR5MQKTZRS2PRAVCNFSM6AAAAABLY7NY6SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEOJRGM3DANRRGM. You are receiving this because you were mentioned.Message ID: @.***>

BGull00 commented 3 weeks ago

I agree that -inf for minimum potential would likely rarely be useful. It would be easier to require a real minimum potential value too. Simplicity is always nice. The perfect default value for minimum potential would likely be best determined experimentally, but I think those defaults you mentioned would likely work just fine.

Jonathan and I worked on RAVENS FPGA testing in the spring, so we already have some Python scripts that can be used for generating random SNNs (https://bitbucket.org/neuromorphic-utk/neuromorphic-fpga-verification/src/main/testing/src/test_data_generation/NetworkGen.py) and generating random processor tool inputs (https://bitbucket.org/neuromorphic-utk/neuromorphic-fpga-verification/src/main/testing/src/test_data_generation/input_gen.py). Here's the outer pytest that uses these Python files: https://bitbucket.org/neuromorphic-utk/neuromorphic-fpga-verification/src/main/testing/src/test_neuroprocessor.py. They're all a little messy, but most of the functionality is there. I also think most of the code should work for RISP nets as well. The main thing we were missing from this project in my opinion is a way to automatically vary the processor parameters that are loaded from JSON files. Using these, we should be able to run pretty extensive randomized testing.

keegandent commented 3 weeks ago

My opinion on this is that we shouldn't allow it to be -infinity. We should bound it and call it a day. Perhaps the default should be either -max_threshold or maybe -2*max_threshold. Like the threshold, the value can go below this during integration — it's only clamped once integration is done.

I agree that -inf for minimum potential would likely rarely be useful. It would be easier to require a real minimum potential value too. Simplicity is always nice. The perfect default value for minimum potential would likely be best determined experimentally, but I think those defaults you mentioned would likely work just fine.

Fair enough. I would cast my vote for -max_threshold as a default for this parameter, as its signed integer width is exactly what is also necessary for max_threshold + 1. I'm not sure, but you may want or need a rule that min_threshold cannot be below min_potential lest you can never turn a neuron off? I'll leave that to y'all to figure out :)