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

Added support for QONNX Resize node ingestion and tested with tiny UNet model #1122

Merged
merged 21 commits into from
Nov 21, 2024

Conversation

nghielme
Copy link
Contributor

@nghielme nghielme commented Nov 12, 2024

Description

All the necessary parts are added in order to properly ingest QONNX models containing Resize node

Type of change

Extended the capability of QONNX ingestion of hls4ml

  • New feature (non-breaking change which adds functionality)

Tests

I added two tests based on tiny UNet model in test_qonnx.py and one based on a simpler model taken from tiny UNet model. They have been both added in pytest directory.

Checklist

  • I have read the guidelines for contributing.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • I have installed and run pre-commit on the files I edited or added.
  • I have added tests that prove my fix is effective or that my feature works.

@nghielme nghielme requested a review from jmitrevs November 12, 2024 13:33
@JanFSchulte JanFSchulte added the please test Trigger testing by creating local PR branch label Nov 12, 2024
@@ -87,7 +87,7 @@ def transform(self, model, node):
)
for i in range(len(output_map[output])):
key = output + '_cpy' + str(i + 1)
clone_layer.attributes[key].type = node.attributes['result_t']
clone_layer.attributes[key].type = node.get_output_variable().type
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this identical?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here the output of the tests if you run the code with the original line:

    def match(self, node):
        if isinstance(node, (Pooling1D, Pooling2D, GlobalPooling1D, GlobalPooling2D)) and node.get_attr('pool_op') == 'Max':
>           return isinstance(node.get_input_variable().type.precision, XnorPrecisionType) and not isinstance(
                node.get_output_variable().type.precision, XnorPrecisionType
            )
E           AttributeError: 'FixedPrecisionType' object has no attribute 'precision'

hls4ml/backends/fpga/passes/xnor_pooling.py:14: AttributeError

Solved if you update it as proposed.

@jmitrevs jmitrevs added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Nov 12, 2024
hls4ml/converters/onnx_to_hls.py Outdated Show resolved Hide resolved
test/pytest/test_qonnx.py Outdated Show resolved Hide resolved
Copy link
Contributor Author

@nghielme nghielme left a comment

Choose a reason for hiding this comment

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

I think I addressed all the comments. I am noticing the CI/CD pipeline fails in 2 tests, 1 and 2, but apparently they are unrelated to this contribution.

@nghielme nghielme requested a review from jmitrevs November 14, 2024 09:18
@JanFSchulte JanFSchulte added this to the v1.0.0 milestone Nov 15, 2024
@jmitrevs
Copy link
Contributor

Note, this now includes part (but not all) of #1130.

hls4ml/model/layers.py Show resolved Hide resolved
hls4ml/model/optimizer/passes/resize_remove_constants.py Outdated Show resolved Hide resolved
Copy link
Contributor Author

@nghielme nghielme left a comment

Choose a reason for hiding this comment

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

I reviewed all the comments, let me know if there's anything else to address. I see the pipeline failed but again it was due to the large time taken by some tests

@vloncar vloncar merged commit 8505e78 into main Nov 21, 2024
6 of 7 checks passed
@vloncar vloncar deleted the resize_pr branch November 21, 2024 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
please test Trigger testing by creating local PR branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants