Basic SNN functionality in hls4ml (mapped from snntorch)#1470
Basic SNN functionality in hls4ml (mapped from snntorch)#1470bmdillon wants to merge 32 commits into
Conversation
|
pre-commit.ci autofix |
vloncar
left a comment
vloncar
left a comment
There was a problem hiding this comment.
Overall looks good, needs minor changes to keep it in line with the rest of the codebase
| return; | ||
| } | ||
|
|
||
| if (CONFIG_T::decision_rule == snn_decision_rule::binary_logit) { |
There was a problem hiding this comment.
Could the decision_rule be implemented as a function pointer? That way you avoid the excessive if statements (and the valid combination has already been checked in the earlier stages of conversion)
There was a problem hiding this comment.
Could you explain a bit more, where should the functions be? For now I’ve refactored the code in nnet_snnreadout.h to include helper functions, if we need more I can edit further
There was a problem hiding this comment.
I was thinking if there's a way to have something like CONFIG_T::decision_rule() that would be a function pointer to the helper functions that you would set in the config templates. We used a similar approach for matrix-vector multiplication functions in the past. It's not essential for this PR though, it's purely cosmetics
|
pre-commit.ci autofix |
JanFSchulte
added
please test
|
The PR tests show several failures for this new feature. Can you have a look and make sure things work correctly? 
|
These should be fixed now, all run locally. Just a note, I updated utils/config.py, it was setting some layer attributes to default values if they weren't TypeAttribute or RF. If this is the intended behaviour, I can find an alternative fix. |
JanFSchulte
added
please test
vloncar
added
please test
| for node in model.graph.values(): | ||
| if isinstance(node, (IFNeuron, LIFNeuron)) and node.get_attr('window_size') != window_size: | ||
| node.set_attr('window_size', window_size) | ||
| changed = True |
There was a problem hiding this comment.
I don't think this is needed. It is required when changes are made to the execution graph (nodes added/removed/replaced), or if change in an attribute of one node affects some other. But the latter case should be avoided. I don't think that we need to return True in this pass. Can you check if it works without it?
| else: | ||
| if attr.default is not None: | ||
| if attr.name in layer: | ||
| layer_config[attr.config_name] = layer[attr.name] |
There was a problem hiding this comment.
So the idea here is that there may be attributes of the layer that the user configured during model definition, which would then be part of the model when parsed, but that the user can still override during conversion and deployment? Up to this point we considered the semantics of configurable attributes as something purely in hls4ml that controls the behavior of the model in a way that cannot be specified during model definition (in e.g., pytorch, snntorch or kersas), such as the reuse factor or type definitions for non-quantized models. One can argue this breaks the concept of what it means for something to be configurable, but I don't have strong opinions on enforcing it. We kept it hidden (not part of the dict this method produces, but still doable if you really know what you're doing). Thoughts @JanFSchulte @jmitrevs @calad0i @bo3z ?
There was a problem hiding this comment.
This looks good to me. Maybe apply the same change to the other frontends also for consistency.
I would argue that the qkeras autoconfig generator is already doing similar things with the quantizers, i.e., quantizer params defined through config and overridable by the user, though not the same Attribute knobs, I would argue they are similar.
If the user overrides anything inside the default config, they shall know what they are doing. Also, adding the
"default" config here don't change the actual downstream behavior (if this doesn't exist, one can still insert it manually), so also no new footgun.
|
To me this looks ready, assuming tests pass. Let's see if other main devs have issue with the slight change in semantics of configurable attributes. |
4 participants
Description
This PR adds initial PyTorch/SNN support to hls4ml.
It includes conversion support for IF/LIF neuron layers, SNN readout handling, Vivado templates, documentation, tests, and a small example notebook.
Dependencies:
snntorchtorchType of change
Tests
Added pytest coverage for PyTorch SNN parsing, IF/LIF layer handling, scalar/vector beta and threshold values, SNN readout options, and custom PyTorch SNN extension parsing.