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

feat(frontend): add support for np.min and np.max #1025

Merged
merged 1 commit into from
Sep 6, 2024
Merged

Conversation

umut-sahin
Copy link
Contributor

@umut-sahin umut-sahin requested a review from aPere3 August 28, 2024 13:02
@umut-sahin umut-sahin self-assigned this Aug 28, 2024
@cla-bot cla-bot bot added the cla-signed label Aug 28, 2024
@umut-sahin
Copy link
Contributor Author

Btw, I've moved test_min_max.py to test_minimum_maximum.py as they were testing np.minimum and np.maximum, not np.min and np.max. I then introduced test_min_max.py back, so the diff could be a bit confusing. Just ignore the diff and inspect each file separately, it'll be easier.

Copy link
Member

@BourgerieQuentin BourgerieQuentin left a comment

Choose a reason for hiding this comment

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

frontends/concrete-python/concrete/fhe/mlir/context.py Outdated Show resolved Hide resolved
@bcm-at-zama bcm-at-zama self-requested a review August 30, 2024 12:49
Copy link
Contributor

@bcm-at-zama bcm-at-zama left a comment

Choose a reason for hiding this comment

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

Do we also need to update docs/core-features/minmax.md?

@umut-sahin
Copy link
Contributor Author

Do we also need to update docs/core-features/minmax.md?

No. That document explains the primitive comparison between two values, like how it's being done. lt's not for the description of high level features.

For that, we're mimicking numpy and it'll be in the compatibility like @BourgerieQuentin requested.

@bcm-at-zama
Copy link
Contributor

Do we also need to update docs/core-features/minmax.md?

No. That document explains the primitive comparison between two values, like how it's being done. lt's not for the description of high level features.

For that, we're mimicking numpy and it'll be in the compatibility like @BourgerieQuentin requested.

but if now, minmax is "native" in CP, do we need to keep a section to explain how it works? ie, if it's all made for the user, we might not explain, there are a lot of things which are done for the user

@umut-sahin
Copy link
Contributor Author

but if now, minmax is "native" in CP, do we need to keep a section to explain how it works? ie, if it's all made for the user, we might not explain, there are a lot of things which are done for the user

We don't have explicit documentation about np.matmul or np.reshape, why would we have one for min/max? The operation is supported like in numpy, it's enough to document the "primitive" comparison (e.g., strategies for comparing 2 numbers).

@umut-sahin
Copy link
Contributor Author

I've added extensive documentation of how it works in the code via comments. Could you check @aPere3?

@BourgerieQuentin
Copy link
Member

@umut-sahin Check Concrete Python Tests there are a lot of RuntimeError: Transforming FHE boolean ops failed in test_min_max, I don't known why this happening. Must be fixed before merge as well.

Copy link
Collaborator

@aPere3 aPere3 left a comment

Choose a reason for hiding this comment

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

Looks good thanks for the explanations, those are top-notch !

@BourgerieQuentin
Copy link
Member

@slab-ci concrete-python-tests-linux

@umut-sahin umut-sahin force-pushed the feat/min-max branch 4 times, most recently from 6e3c50d to 8b128b0 Compare September 4, 2024 08:43
@umut-sahin umut-sahin force-pushed the feat/min-max branch 13 times, most recently from 1f6a077 to 3e44083 Compare September 5, 2024 13:54
@umut-sahin umut-sahin merged commit d3dfdcd into main Sep 6, 2024
31 of 33 checks passed
@umut-sahin umut-sahin deleted the feat/min-max branch September 6, 2024 06:39
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