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

Fix RemoveIdentityOps not correctly handling ops following fork-nodes #87

Merged
merged 114 commits into from
Sep 12, 2024

Conversation

iksnagreb
Copy link
Contributor

@iksnagreb iksnagreb commented Nov 15, 2023

Fixes the issue reported here #86, by adding a missing test for the producer node being a fork node. Added some (probably too extensive) documentation and a new check to avoid removing join-node operations by accident (currently not possible, but this was not really documented).

Closes #86

maltanar and others added 27 commits August 13, 2024 10:50
…t_conv_lowering_convmnist

Improve/test conv lowering convmnist
random input generated with seed=42 was causing a major difference
in Conv_13_out0 for no apparent reason (probably float / numerical
related)
…eature/convlower_qnt

Preserve weight quantizer while lowering convolutions
…eature/test_a2q_nets

Add easy fetch of accumulator-aware quantized (A2Q) CIFAR-10 models for testing
…_commutative_inputs

Add cleanup transformation sorting inputs of commutative operations
@maltanar
Copy link
Collaborator

LGTM. Thanks @iksnagreb !
I took the liberty to add the key commit from #137 by @jurevreca12 and expanded the testcases for removal of identity ops to cases with Identity nodes and forks before nodes to be removed. This revealed a bug in the ModelWrapper.is_fork_node() check which ignored top-level outputs, so added a small fix for that and the corresponding is_join_node as well.

@maltanar maltanar merged commit 88a679a into fastmachinelearning:main Sep 12, 2024
5 checks passed
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.

RemoveIdentityOps does not correctly handle identity operations following fork-nodes
9 participants