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.

Sign up for GitHub

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

Jump to bottom

chore: fix where which use boolean params #863

Merged
merged 1 commit into from
Oct 28, 2024
Merged

chore: fix where which use boolean params #863

merged 1 commit into from
Oct 28, 2024

Conversation

jfrery
Copy link
Collaborator

@jfrery jfrery commented Sep 6, 2024

closes #862

@cla-bot cla-bot bot added the cla-signed label Sep 6, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 6, 2024

⚠️ Known flaky tests have been rerun ⚠️

One or several tests initially failed but were identified as known flaky. tests. Therefore, they have been rerun and passed. See below for more details.

Failed tests details

Known flaky tests that initially failed:

  • tests/torch/test_compile_torch.py::test_compile_torch_or_onnx_conv_networks[True-True-CNN-relu]

@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 6, 2024

Coverage passed ✅

Coverage details 

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

60 files skipped due to complete coverage.

andrei-stoian-zama
andrei-stoian-zama reviewed Sep 6, 2024
View reviewed changes
src/concrete/ml/pytest/torch_models.py
Returns:
torch.Tensor: The output tensor after applying the where operation.
"""
y = torch.where(self.fc_tot, x, 0.0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

the condition here is not done on the encrypted variable. is this the case we should test ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure I understand. self.fc_tot is the condition, a constant, not encrypted and x is the input encrypted tensor that will be 0 where condition matches?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should also test the case when the condition is on an encrypted variable

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Isn't this the case? x is encrypted and self.fc_tot isn't. Do you mean "is not on an encrypted variable?"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok sorry you are right the conditions are in clear we are just selecting a part of the encrypted tensor.

@jfrery jfrery marked this pull request as ready for review September 17, 2024 10:55
@jfrery jfrery requested a review from a team as a code owner September 17, 2024 10:55
@jfrery jfrery merged commit b307ca5 into main Oct 28, 2024
14 checks passed
@jfrery jfrery deleted the chore/fix_where branch October 28, 2024 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrei-stoian-zama andrei-stoian-zama andrei-stoian-zama approved these changes

@kcelia kcelia kcelia approved these changes

Assignees
No one assigned
Labels
cla-signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Torch.where is not correctly supported
3 participants
@jfrery @kcelia @andrei-stoian-zama