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 RoundAndClipThresholds Transformation #1030

Merged
merged 8 commits into from
Oct 7, 2024

Conversation

iksnagreb
Copy link
Contributor

@iksnagreb iksnagreb commented Apr 6, 2024

See #978 for some explanation of what it wrong with this transformation. See #875 for more context.

  • Rework the RoundAndClipThresholds transformation to correctly respect the range of the input data type (the quantization annotation of the input) and the container data type of the thresholds tensor. Clearly defines the type-casting/type-promotion behavior to always produce integer thresholds coded as float32 initializer tensors.
  • Add test-cases for the RoundAndClipThresholds transformation, testing rounding, clipping and different data type combinations for large bit-width integers around INT24 (where the representability issues start to occur).
  • Validate changes against the MobileNet v1 test-case (the one failing after the initial, naive fix)
  • Run all quick-tests
    - [ ] Validate against the other end2end tests
  • Validate against our transformer use case
    - [ ] Build finn-examples

Closes #978

@iksnagreb
Copy link
Contributor Author

Hm, while I can successfully run the mobilenet and the cybersecurity end2end tests, the bnn-pynq tests fail immediately at the export step, this seems not to be my fault (at least it is unrelated to this PR, maybe I am missing something else or messed up some dependency?):

tests/end2end/test_end2end_bnn_pynq.py, line 459
      def test_export(self, topology, wbits, abits, board):
E       fixture 'topology' not found

Any hints on this error?

Regarding building the finn-examples: It seems these have not been updated to the recent finn release yet? Will this happen in the near future or should we skip this test for now?

@auphelia
Copy link
Collaborator

Hi @iksnagreb,

Yes, we restructured the bnn pynq tests and they are not callable with pytest -k bnn_pynq anymore. You could run one of the markers, e.g pytest -m sanity_bnn.
If you like, I can take it from here and do the testing (apart from your transformer use case). We are currently in the process of updating the finn-examples to the new flow.

@iksnagreb
Copy link
Contributor Author

Ok, then I will skip doing the end2end and finn-examples tests now and focus on our transformer model. My dummy model already built successfully, but I would like to test the "real" model as well, just to be sure. I will also mark this as ready for review now.

@iksnagreb iksnagreb marked this pull request as ready for review April 26, 2024 09:55
@auphelia
Copy link
Collaborator

Thanks @iksnagreb !

@auphelia auphelia marked this pull request as draft September 26, 2024 14:37
@auphelia auphelia marked this pull request as ready for review September 27, 2024 14:37
@auphelia auphelia merged commit 8da09eb into Xilinx:dev Oct 7, 2024
2 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.

2 participants