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

chore: remove manual set of multi and mono strategy #256

Merged

Conversation

RomanBredehoft
Copy link
Collaborator

Recent Concrete- Python integration :

  • fixed the multi strategy problem with linear models
  • made multi strategy default in Configuration instances

refs https://github.com/zama-ai/concrete-ml-internal/issues/3889
closes https://github.com/zama-ai/concrete-ml-internal/issues/3862
closes https://github.com/zama-ai/concrete-ml-internal/issues/3860

@cla-bot cla-bot bot added the cla-signed label Sep 18, 2023
@RomanBredehoft RomanBredehoft force-pushed the chore/remove_multi_and_mono_strategy_manual_set_3860 branch from 061b108 to 23d5cc5 Compare September 18, 2023 09:47
@RomanBredehoft RomanBredehoft marked this pull request as ready for review September 18, 2023 11:57
@RomanBredehoft RomanBredehoft requested a review from a team as a code owner September 18, 2023 11:57
@andrei-stoian-zama
Copy link
Collaborator

Will this not impact @kcelia's PR where she needs to set MONO so CP doesn't crash ?

fd0r
fd0r previously approved these changes Sep 18, 2023
@fd0r
Copy link
Collaborator

fd0r commented Sep 18, 2023

I'm guessing this mono/multi issue only affects KNN so she can just set MONO in KNN dedicated test suite no?

@RomanBredehoft
Copy link
Collaborator Author

ah yes forgot to mention, this will conflict a bit with @kcelia's PR as I indeed removed force_mono_parameter_in_configuration (which she needs to use)

so either I wait for her the merge and then I rebase on main (which I think is best), or I put back the function with #pragma: no-cover in it (else pylint will probably complain that it's not used) cc @andrei-stoian-zama @fd0r

@RomanBredehoft RomanBredehoft changed the title chore: remove manual set of multi and mono strategy chore: remove manual set of multi and mono strategy [ON HOLD] Sep 18, 2023
@RomanBredehoft RomanBredehoft marked this pull request as draft September 18, 2023 13:38
@kcelia
Copy link
Collaborator

kcelia commented Sep 18, 2023

Thanks @RomanBredehoft !

@RomanBredehoft RomanBredehoft force-pushed the chore/remove_multi_and_mono_strategy_manual_set_3860 branch from 23d5cc5 to 2ea6018 Compare September 21, 2023 14:46
@RomanBredehoft RomanBredehoft changed the title chore: remove manual set of multi and mono strategy [ON HOLD] chore: remove manual set of multi and mono strategy Sep 21, 2023
@RomanBredehoft RomanBredehoft force-pushed the chore/remove_multi_and_mono_strategy_manual_set_3860 branch from 2ea6018 to 96eb5f6 Compare September 22, 2023 09:08
@RomanBredehoft RomanBredehoft force-pushed the chore/remove_multi_and_mono_strategy_manual_set_3860 branch from 96eb5f6 to 9de1b2d Compare September 22, 2023 11:42
@github-actions
Copy link

Coverage passed ✅

Coverage details

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

51 files skipped due to complete coverage.

@RomanBredehoft RomanBredehoft marked this pull request as ready for review September 22, 2023 13:06
Copy link
Collaborator

@jfrery jfrery left a comment

Choose a reason for hiding this comment

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

Nice clean up! I thought KNN was still needing mono params tho?

Copy link
Collaborator

@kcelia kcelia left a comment

Choose a reason for hiding this comment

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

alright, thanks

@RomanBredehoft RomanBredehoft merged commit 3b2d8c9 into main Sep 22, 2023
8 checks passed
@RomanBredehoft RomanBredehoft deleted the chore/remove_multi_and_mono_strategy_manual_set_3860 branch September 22, 2023 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants