Skip to content

AddCLSToken#1575

Open
ollycassidy13 wants to merge 4 commits into
Xilinx:devfrom
ollycassidy13:feature/AddCLSToken
Open

AddCLSToken#1575
ollycassidy13 wants to merge 4 commits into
Xilinx:devfrom
ollycassidy13:feature/AddCLSToken

Conversation

@ollycassidy13
Copy link
Copy Markdown

Prepends a learned class token, supplied through cls_data, to each input sequence of patch tokens. The class token and patch tokens are transferred as SIMD-wide folds of ELEM_WIDTH-bit elements.

Copy link
Copy Markdown
Collaborator

@STFleming STFleming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Olly looks good! Just some small questions / suggestions.

inputs = {}
for i, inp in enumerate(node.input):
nbits = self.get_instream_width(i)
if nbits == 0:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't a 0-width input throw an assertion of some kind? Is there a use case where a 0-width input is needed?

patches = np.arange(12, dtype=np.float32).reshape(1, 3, 4)
expected = np.concatenate([cls_values, patches], axis=1)

ret = execute_onnx(model, {"patches": patches})
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've been caught out a few time with tests that rely on particular input names. It might be better to extract the name to use from the model itself {model.graph.input[0].name : patches} sometimes FINN transformations can rename this inputs and it silently fails.

node.input,
node.output,
domain="finn.custom_op.fpgadataflow." + impl_style,
name=node.name,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@auphelia is this okay? Or is it breaking any naming assumptions downstream?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be redundant

model = model.transform(GiveUniqueNodeNames())

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants