Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TypeError fixes in converter.py->ConvertConcatenate #230

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

julvanu
Copy link

@julvanu julvanu commented Dec 14, 2022

I wanted to convert a Keras model using the nengo_dl Converter class as proposed in the docu:
https://www.nengo.ai/nengo-dl/examples/keras-to-snn.html#Converting-a-Keras-model-to-a-Nengo-network

Thereby, I ran into two bugs so far. The issue seems to be, that nengo_dl Converter is not prepared for a Concatenate layer to get a list of length 1 as input. Here is my approach to fix it.

  1. self.input_shape in ConvertConcatenate returns EITHER a list of tuples OR just a tuple, though the convert function in ConvertConcatenate assumes that what is returned is always a list of tuples. Therefore, in specific cases as in mine you get a TypeError.

  2. If I get it right, this for loop at line 1242 assumes self.layer.input to always be a list of KerasTensors. Though, if the Concatenate layer gets only one KerasTensor as input, self.layer.input is not a list but just that KerasTensor. Keras layers work on symbolic inputs/outputs meaning their "KerasTensor" do not hold actual data and therefore do not implement functions like "__len__". That is why in that case we gete a TypeError as well.

@julvanu julvanu changed the title Julvanu TypeError fixes in converter.py->ConvertConcatenate Dec 14, 2022
…ways be a list, though for one element it is not
@drasmuss
Copy link
Member

Hi @julvanu, thanks for the bug fix! Could you add a test case for converting a concatenate layer with a single input? You can see the current concatenate test here https://github.com/nengo/nengo-dl/blob/master/nengo_dl/tests/test_converter.py#L136, probably we just want to parametrize that to add a single input case. I can also add that in myself when I get to reviewing it if you don't get the chance. We're a little backed up on NengoDL work currently, but I'd anticipate getting to this early in the new year.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants