feat: add multi-output support #396

fd0r · 2023-11-24T13:10:10Z

closes https://github.com/zama-ai/concrete-ml-internal/issues/4139

Makefile

benchmarks/deep_learning.py

RomanBredehoft

looks good ! I only have a few open questions remaining, but would be fine for me if we merge it like this , again, thanks a lot for this integration !

RomanBredehoft · 2023-12-07T14:15:13Z

src/concrete/ml/sklearn/base.py

@@ -1116,9 +1115,11 @@ def quantize_input(self, X: numpy.ndarray) -> numpy.ndarray:
        assert isinstance(q_X, numpy.ndarray)
        return q_X

-    def dequantize_output(self, q_y_preds: numpy.ndarray) -> numpy.ndarray:
+    def dequantize_output(self, *q_y_preds: numpy.ndarray) -> numpy.ndarray:


just realized here, we have a signature change here : dequantize_output is a common method between all built-in models, this means it 'inherits' from the BaseEstimator method, which has the following signature / docstring :

@abstractmethod def dequantize_output(self, q_y_preds: numpy.ndarray) -> numpy.ndarray: """De-quantize the output. This step ensures that the fit method has been called. Args: q_y_preds (numpy.ndarray): The quantized output values to de-quantize. Returns: numpy.ndarray: The de-quantized output values. """

what should we do then here ? I feel like this signature change is necessary (I guess QNNs could be multi-output right ?) but dequantize_output for linear / tree models should stay like it is right now

I'm actually surprised mypy did not complain here, I thought it would point out that the method's signature has changed .. ?

so yes, not sure what could be done here, it probably looks fine to just keep it liks this but maybe add a new docstring to better indicate that the signature is a bit different ?

So actually if I understand correctly def f(arg) and def f(*arg) aren't incompatible since one is just a superset of the other.

I am not sure if it makes sense to change it for builtin models too, but a relevant question indeed.

if def f(*arg) is properly considered a super-set of def f(arg) in signatures then I think it's fine !

RomanBredehoft · 2023-12-07T14:16:03Z

pyproject.toml

@@ -34,7 +34,7 @@ readme = "README.md"
 # Investigate if it is better to fix specific versions or use lower and upper bounds  
 # FIXME: https://github.com/zama-ai/concrete-ml-internal/issues/2665
 python = ">=3.8.1,<3.11"
-concrete-python = "2.5.0-rc1 "
+#concrete-python = "2023.11.5"


I think that's the main issue with nightly, I believe we don't want them to be public, only rc should

tests/torch/test_compile_torch.py

RomanBredehoft

Thanks, all is good for me !

tests/quantization/test_compilation.py

tests/sklearn/test_sklearn_models.py

andrei-stoian-zama · 2023-12-08T13:10:24Z

src/concrete/ml/quantization/quantized_module.py

            for (_, layer) in self.quant_layers_dict.values():
                layer.debug_value_tracker = None
-            return result, debug_value_tracker
+            # De-quantize the output predicted values
+            y_pred = self.dequantize_output(*to_tuple(q_y_pred))


you can keep the debug values here as well, the computation is still done on integers

RomanBredehoft

sorry but if we add new flaky marks to pytest, better to create an issue + add a fixme along these marks !

fd0r · 2023-12-08T14:54:42Z

Indeed my bad, I'll create the issue and add the FIXME.

RomanBredehoft

thanks a lot for this !

github-actions · 2023-12-11T10:46:26Z

Coverage passed ✅

Coverage details

---------- coverage: platform linux, python 3.8.18-final-0 -----------
Name    Stmts   Miss  Cover   Missing
-------------------------------------
TOTAL    6430      0   100%

51 files skipped due to complete coverage.

jfrery

Thanks!

cla-bot bot added the cla-signed label Nov 24, 2023

fd0r force-pushed the multi_output branch 2 times, most recently from 5f5a4de to 1a7c302 Compare November 24, 2023 13:44

jfrery reviewed Nov 24, 2023

View reviewed changes

Makefile Outdated Show resolved Hide resolved

fd0r force-pushed the multi_output branch 15 times, most recently from 82d7d9f to 6fb8011 Compare November 29, 2023 18:41

jfrery reviewed Nov 30, 2023

View reviewed changes

benchmarks/deep_learning.py Outdated Show resolved Hide resolved

fd0r force-pushed the multi_output branch 7 times, most recently from 30aa027 to 9543d4b Compare December 5, 2023 15:56

fd0r marked this pull request as ready for review December 5, 2023 17:49

fd0r requested a review from a team as a code owner December 5, 2023 17:49

fd0r force-pushed the multi_output branch from 9543d4b to 8b40dfd Compare December 6, 2023 08:19

fd0r force-pushed the multi_output branch from 939d525 to 0802a32 Compare December 7, 2023 14:12

RomanBredehoft previously approved these changes Dec 7, 2023

View reviewed changes

fd0r dismissed RomanBredehoft’s stale review via bb32651 December 7, 2023 14:25

fd0r force-pushed the multi_output branch from 0802a32 to bb32651 Compare December 7, 2023 14:25

RomanBredehoft previously approved these changes Dec 7, 2023

View reviewed changes

feat: add multi-output support

7a164ed

fd0r dismissed RomanBredehoft’s stale review via 7a164ed December 7, 2023 14:46

fd0r force-pushed the multi_output branch from bb32651 to 7a164ed Compare December 7, 2023 14:46

RomanBredehoft previously approved these changes Dec 7, 2023

View reviewed changes

fd0r requested a review from andrei-stoian-zama December 7, 2023 14:48

andrei-stoian-zama previously approved these changes Dec 7, 2023

View reviewed changes

fd0r dismissed stale reviews from andrei-stoian-zama and RomanBredehoft via 91f0d5b December 8, 2023 10:38

fd0r requested review from andrei-stoian-zama and RomanBredehoft December 8, 2023 12:24

RomanBredehoft reviewed Dec 8, 2023

View reviewed changes

tests/quantization/test_compilation.py Show resolved Hide resolved

RomanBredehoft reviewed Dec 8, 2023

View reviewed changes

tests/sklearn/test_sklearn_models.py Show resolved Hide resolved

andrei-stoian-zama previously approved these changes Dec 8, 2023

View reviewed changes

RomanBredehoft requested changes Dec 8, 2023

View reviewed changes

fd0r dismissed andrei-stoian-zama’s stale review via 98e1586 December 8, 2023 14:59

fd0r force-pushed the multi_output branch from 91f0d5b to 98e1586 Compare December 8, 2023 14:59

fd0r requested a review from RomanBredehoft December 10, 2023 23:23

fix: mark flaky tests due to issue in simulation

71cf9b7

fd0r force-pushed the multi_output branch from 98e1586 to 71cf9b7 Compare December 11, 2023 09:33

RomanBredehoft approved these changes Dec 11, 2023

View reviewed changes

jfrery approved these changes Dec 11, 2023

View reviewed changes

fd0r merged commit 4f67883 into main Dec 11, 2023
9 checks passed

fd0r deleted the multi_output branch December 11, 2023 15:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add multi-output support #396

feat: add multi-output support #396

fd0r commented Nov 24, 2023 •

edited

Loading

RomanBredehoft left a comment

RomanBredehoft Dec 7, 2023

fd0r Dec 7, 2023

RomanBredehoft Dec 7, 2023

RomanBredehoft Dec 7, 2023

RomanBredehoft left a comment

andrei-stoian-zama Dec 8, 2023

RomanBredehoft left a comment

fd0r commented Dec 8, 2023

RomanBredehoft left a comment

github-actions bot commented Dec 11, 2023

jfrery left a comment

feat: add multi-output support #396

feat: add multi-output support #396

Conversation

fd0r commented Nov 24, 2023 • edited Loading

RomanBredehoft left a comment

Choose a reason for hiding this comment

RomanBredehoft Dec 7, 2023

Choose a reason for hiding this comment

fd0r Dec 7, 2023

Choose a reason for hiding this comment

RomanBredehoft Dec 7, 2023

Choose a reason for hiding this comment

RomanBredehoft Dec 7, 2023

Choose a reason for hiding this comment

RomanBredehoft left a comment

Choose a reason for hiding this comment

andrei-stoian-zama Dec 8, 2023

Choose a reason for hiding this comment

RomanBredehoft left a comment

Choose a reason for hiding this comment

fd0r commented Dec 8, 2023

RomanBredehoft left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 11, 2023

Coverage passed ✅

jfrery left a comment

Choose a reason for hiding this comment

fd0r commented Nov 24, 2023 •

edited

Loading