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: Add array comparison methods. #60

Merged
merged 4 commits into from
Aug 27, 2024
Merged

Conversation

jcabrero
Copy link
Member

This PR introduces support for Array Comparison Methods i.e., ==, !=, >, >=, <, <=. This enables certain use cases where users where trying to use the comparison operators with Nillion.

Apart from this it also:

  • It slightly improves the broadcasting tests as there were certain issues I wasn't able to spot.
  • It bumps the version to create a new release including these features.

Returns:
NadaArray: A boolean array representing the element-wise inequality comparison result.
"""
return self.__comparison_operator(value, lambda x, y: ~(x == y))
Copy link
Member

Choose a reason for hiding this comment

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

I am impressed this is passing the tests. Afaik there is no boolean operations like negation in Nada.

Copy link
Member

Choose a reason for hiding this comment

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

Just checked in the code base and we already support the not operation. We should update the docs then.

Copy link
Member Author

Choose a reason for hiding this comment

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

I know, I was also not fully aware of how to implement this, and had to look through the code base to find it.

Copy link
Contributor

@mathias-nillion mathias-nillion left a comment

Choose a reason for hiding this comment

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

I think we might be able to use .apply here instead of the comparison operator. If not, LGTM

"""
Perform element-wise comparison with broadcasting.

NOTE: Specially for __eq__ and __ne__ operators, the result expected is bool.
Copy link
Contributor

@mathias-nillion mathias-nillion Aug 27, 2024

Choose a reason for hiding this comment

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

Is this the expected result? In numpy, __ eq __ and __ ne __ are broadcasted
I believe this implementation matches that behaviour, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, in NumPy, those two operations are indeed broadcasted. However, there's an unusual behavior when you try to do array_a == array_b. NumPy evaluates the boolean value of each element, as these operations are expected to result in either True or False. For instance, it will determine whether a SecretInteger is True or False—which always turns out to be True.

@@ -390,6 +390,106 @@ def __imatmul__(self, other: Any) -> "NadaArray":
"""
return self.matmul(other)

def __comparison_operator(self, value: Any, operator: Callable) -> "NadaArray":
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use .apply here?

@jcabrero jcabrero merged commit d95eb5d into main Aug 27, 2024
4 checks passed
@jcabrero jcabrero deleted the feat/add_array_comparison branch August 28, 2024 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants