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.

Sign up for GitHub

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

[Lookup] Relax input datatype constraints #1267

Merged
merged 5 commits into from
Feb 20, 2025
Merged

[Lookup] Relax input datatype constraints #1267

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6ace464
[Lookup] Relax input datatype constraints
iksnagreb May 30, 2024
3de81d0
[Lookup] Reintroduce prior assertion as warning and fix type comparison
iksnagreb Jan 29, 2025
15c6e38
[Lookup] Test data should also be read as float32 container type
iksnagreb Feb 14, 2025
e65da62
Merge remote-tracking branch 'upstream/dev' into fix/lookup
auphelia Feb 20, 2025
32fbff0
[LookUp] With npy type being float we can use parent class method for…
auphelia Feb 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions src/finn/custom_op/fpgadataflow/hls/lookup_hls.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@

import numpy as np
import os
import warnings
from math import ceil, log2
from qonnx.core.datatype import DataType

Expand Down Expand Up @@ -97,7 +98,7 @@ def read_npy_data(self):
packed_bits = self.get_instream_width()
packed_hls_type = "ap_uint<%d>" % packed_bits
elem_hls_type = dtype.get_hls_datatype_str()
npy_type = "int64_t"
npy_type = "float"
npy_in = "%s/input_0.npy" % code_gen_dir
self.code_gen_dict["$READNPYDATA$"] = []
self.code_gen_dict["$READNPYDATA$"].append(
Expand Down Expand Up @@ -273,7 +274,18 @@ def execute_node(self, context, graph):
)

inp = context[node.input[0]]
assert inp.dtype == np.int64, "Inputs must be contained in int64 ndarray"

# Make sure the input has the right container datatype
if inp.dtype is not np.float32:
# Issue a warning to make the user aware of this type-cast
warnings.warn(
f"{node.name}: Changing input container datatype from "
f"{inp.dtype} to {np.float32}"
)
# Convert the input to floating point representation as the
# container datatype
inp = inp.astype(np.float32)

assert inp.shape == exp_ishape, """Input shape doesn't match expected shape."""
export_idt = self.get_input_datatype()
odt = self.get_output_datatype()
Expand Down
16 changes: 12 additions & 4 deletions src/finn/custom_op/fpgadataflow/rtl/streamingfifo_rtl.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,10 +133,18 @@ def execute_node(self, context, graph):
elif mode == "rtlsim":
code_gen_dir = self.get_nodeattr("code_gen_dir_ipgen")
# create a npy file for the input of the node
assert (
str(inp.dtype) == "float32"
), """Input datatype is
not float32 as expected."""

# Make sure the input has the right container datatype
if inp.dtype is not np.float32:
Copy link
Collaborator

Choose a reason for hiding this comment

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

How come you needed to add this in StreamingFIFO as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, not exactly sure, I have seen this assertion fail in combination with Lookup. And I think, at least for simulation, it is indeed to restrictive in general: Almost always we assume tensors to be stored in float32 containers and usually QONNX attaches a datatype annotation (to represent the actual, quantized datatype) to this. However, some transformations, operators and also some numpy functions used by the infrastructure do not always keep these two concept as cleanly separated as they should (some numpy functions for example implicitly default to float64, we have already seen this as part of the RoundAndClipThresholds issue..., in this case, Lookup/Gather wants some int64 type, I guess). Whether the whole float32 container type thing really make sense or should be reworked to respect more numpy types is another discussion, but for now I think, anytime we are asserting something about the dtype, i.e., the container type, we should relax this to some warning alongside .astype(np.float32) to restore the expected behavior and just check whether the simulation still yields the expected results.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Warnings have a tendency to be overlooked and this code changes the model like a transformation would. If I have your ok, I would rather suggest a temporary interpretation of the dtype as float32 for the execution but afterwards reverting this back to what the tensor dtype was. For the LookUp layer, I am ok with these changes for now because it is more contained and I believe that layer needs a bigger refactoring soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should not change the model, this is only part of the execute_node rtlsim infrastructure and if you look at the output side, it already does something similar, just without any warning/error:

finn/src/finn/custom_op/fpgadataflow/rtl/streamingfifo_rtl.py

Line 174 in 15c6e38

output = np.asarray([output], dtype=np.float32).reshape(*oshape)

And likewise the cppsim python/numpy fallback:

finn/src/finn/custom_op/fpgadataflow/rtl/streamingfifo_rtl.py

Line 131 in 15c6e38

output = np.asarray([output], dtype=np.float32).reshape(*exp_shape)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh yes, I see! Thanks, I mistook inp for the tensor itself and not the input values.

# Issue a warning to make the user aware of this type-cast
warnings.warn(
f"{node.name}: Changing input container datatype from "
f"{inp.dtype} to {np.float32}"
)
# Convert the input to floating point representation as the
# container datatype
inp = inp.astype(np.float32)

expected_inp_shape = self.get_folded_input_shape()
reshaped_input = inp.reshape(expected_inp_shape)
if DataType[self.get_nodeattr("dataType")] == DataType["BIPOLAR"]:
Expand Down
Loading