-
Notifications
You must be signed in to change notification settings - Fork 148
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
Testing/rounding feature on cml trees 4104 #392
Conversation
5e89ffd
to
692c4ae
Compare
99cd997
to
66f6c41
Compare
b933eeb
to
74d8b97
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had a quick look. Great job! I have a few comments.
17ef517
to
f2a080e
Compare
@RomanBredehoft is it ok for you ? |
bca5660
to
4c3cdb1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine with this PR. Thanks Celia for taking all the comments into account. Let's continue this with truncate now!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! thanks
0272194
0272194
to
ce2bc47
Compare
the maximum_bit (of the circuit without rounding) = maximum_bitwidth (of the circuit with rounding) Remove the test with rounding for n_bits=11
ce2bc47
to
586d7db
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me , thanks a lot !
Coverage passed ✅Coverage details
|
The MSB is set to a value of 1 by default in all our tree-based models. This setting should give the fastest results without decreasing the model's performance. You can review the results of the experiments here.
1. Using rounding_bit_padding and auto-rounding:
2. Using truncate and auto-truncate:
3. Computing the required bitwidth and LSB via the ONNX Graph: (WORKAROUNG)
lsbs_to_remove(Optional[int])
to existing ONNX nodesIn the current implementation, we have opted for approach 3 + adding a flag to existing nodes in the ONNX graph.
From time to time, we might get an overflow, but the issue problem is not reproducible.
eg: pytest tests/sklearn/test_sklearn_models.py::test_rounding_consistency[6-RandomForestClassifier1] --forcing_random_seed 8576543745180669800 --randomly-dont-reset-seed
The issue with the parallelization shared resources in concrete.fhe.extensions.round_bit_pattern.py is fixed.
closes https://github.com/zama-ai/concrete-ml-internal/issues/4157
ref Add new test for level 2
ref Add a test to check if graphs are identical after and before serialization
ref Control the maximum bitwidth of a circuit with rounding