Configurable activations #194
Conversation
…t instead of a string is now supported.
|
Build seems to be failing for a totally unrelated reason (the runner cannot find the "build" directory). I marked this as ready for review even though the build is not passing. |
sdatkinson
left a comment
sdatkinson
left a comment
There was a problem hiding this comment.
Worried about deallocation. Will chat.
NAM/activations.cpp
Outdated
| return _activations[name]; | ||
| } | ||
|
|
||
| nam::activations::Activation* nam::activations::Activation::get_activation(const nlohmann::json& activation_config) |
There was a problem hiding this comment.
I can live with this for now, but I'd prefer something more typed than a "mystery JSON" being the input object. Something like the model factory/registry pattern feels like it might work here.
NAM/wavenet.cpp
Outdated
| const int kernel_size = layer_config["kernel_size"]; | ||
| const auto dilations = layer_config["dilations"]; | ||
| const std::string activation = layer_config["activation"].get<std::string>(); | ||
| const nlohmann::json activation_config = layer_config["activation"]; |
There was a problem hiding this comment.
How about we parse this up into the "new-style" here? If it's a string, then put it into a dict under "type".
NAM/wavenet.h
Outdated
| , _input_mixin(condition_size, (gating_mode != GatingMode::NONE) ? 2 * bottleneck : bottleneck, false) | ||
| , _1x1(bottleneck, channels, groups_1x1) | ||
| , _activation(activations::Activation::get_activation(activation)) // needs to support activations with parameters | ||
| , _activation(activations::Activation::get_activation(activation_config)) // now supports activations with parameters |
There was a problem hiding this comment.
Nit: Can remove the comment :)
tools/test/test_activations.cpp
Outdated
| nlohmann::json str_activation = "ReLU"; | ||
| auto act = nam::activations::Activation::get_activation(str_activation); | ||
| assert(act != nullptr); | ||
| // Don't delete global activation objects |
There was a problem hiding this comment.
[C] Is there something risky/tricky here w/ memory leaks?
Do we need to implement non-default destructors for objects that own activations? If we do, then that might be bad enough that I'd want to do something to avoid it.
…s do not have to manage memory allocated by it.
… and created a method to convert from string or JSON to ActivationConfig so parsing happens sooner.
|
Issues raised regarding memory leaks were fixed by making all activations from get_activation a shared_ptr. Also updated the get_activation function to work with an ActivationConfig object instead of a JSON directly. |
| class Activation; | ||
|
|
||
| // Strongly-typed activation type enum | ||
| enum class ActivationType |
There was a problem hiding this comment.
This enum is good for now.
In the future, I may think about reworking this so that other libraries can build on & extend it.
| ActivationType type; | ||
|
|
||
| // Optional parameters (used by specific activation types) | ||
| std::optional<float> negative_slope; // LeakyReLU, PReLU (single) |
There was a problem hiding this comment.
TIL std::optional. I may rework some of the other spots in the code to use this :)
| unsigned long actual_channels = static_cast<unsigned long>(matrix.rows()); | ||
|
|
||
| // Prepare the slopes for the current matrix size | ||
| std::vector<float> slopes_for_channels = negative_slopes; |
There was a problem hiding this comment.
Worried that this might not be real-time safe.
|
|
||
| static void test_all_simple_types() | ||
| { | ||
| // Test that all simple activation types work |
There was a problem hiding this comment.
Nit: Looks like we'll have to remember to come back here if we add an activation. Would be nice to have something that automatically enumerates what's there.
2 participants
Since some activations now require parameters, I updated the code for
get_activationto take in either a string or a JSON object. In the former case it works as previously for backward compatibility, but when passed a JSON object it will now parse the object for activation parameters and initialize them properly. This is supported for all activations that support parameters in the codebase (leaky ReLU, PReLU, leaky hard tanh).A potentially undesired update is how PReLU handles multiple channels when it is initialized with a single parameter. I updated the code to broadcast the parameter to all channels. Right now it's being done dynamically just for testing, but if the feature is not required and we just want it to fail when the input has more channels than the PReLU activation, we can do that too.Developed with support and sponsorship from TONE3000