Converter improvements #137
Conversation
|
Just a heads up, you might want to start from this branch #134, as it has some of the changes you're adding already. It also has a more efficient convolutional bias implementation (I forgot that wasn't in master yet), which would be good to benchmark against the inference_only version to see if there is still an advantage. |
|
I did a quick check of the new bias code in #130 on my network, and it appears to be as fast. I've put my old work in https://github.com/nengo/nengo-dl/tree/converter-hunse-inf-only-bias just in case we want it later, and rebased this onto #134. |
hunse
force-pushed
the
converter-hunse
branch
2 times, most recently
from
94ec5e0 to
e3aa159
Compare
drasmuss
force-pushed
the
converter-hunse
branch
from
e3aa159 to
da596f3
Compare
There was a problem hiding this comment.
Added some fixups.
With respect to the "Better error for unconverted input tensor", my thinking was that since this isn't an error that users should ever be running into, we don't really need to provide extra code for a pretty error message. So I changed it so that it just directly raises the relevant error (a KeyError if node_id doesn't exist) at the appropriate line in the code, which should make that easier to debug (rather than getting that pre=None error), without adding any new logic/parameters/tests.
The other change is just simplifying the test somewhat for the sorting fix.
If those look good to you, will merge!
| return None | ||
| else: | ||
| raise ValueError("Input tensor %s has not been converted" % (tensor)) | ||
| return self.converter.layer_map[input_layer][input_node_id][input_tensor_id] |
There was a problem hiding this comment.
It was my impression that sometimes this function needed to be called on nodes that aren't in the layer map, and return None in that case to indicate that. But if that's not necessary, this is certainly much simpler!
There was a problem hiding this comment.
That only happens in one case, in ConvertInputs, so I just changed it to directly handle that case in that function, rather than handling it in this generic method.
| # if this input layer has an input obj, that means it is a passthrough | ||
| # (so we just return the input) | ||
| output = self.get_input_obj(node_id) | ||
| except KeyError: |
There was a problem hiding this comment.
Ahh, I see, this is the one place we needed that, so then we just catch the error. Works for me, as long as there's not any risk with catching some other KeyError by mistake.
drasmuss
force-pushed
the
converter-hunse
branch
from
da596f3 to
df05809
Compare
This is no longer required, and is problematic for models that have an output that is used other places in the model (since the sorting puts all outputs at the end). Also ensure a better error for unconverted input tensor
drasmuss
force-pushed
the
converter-hunse
branch
from
df05809 to
b8394f6
Compare
Some improvements to get the Keras converter working with my network.
source_tensorlist. This is no longer required, and is problematic for models that have an output that is used other places in the model (since the sorting puts all outputs at the end).Use a single bias connection ininference_onlymode. We don't need to worry about proper bias sharing since we're not training them. It's much faster to use one connection to do all the biases in this case.TODO: