Skip to content

feat: add shuffle feature to array with power of two size#69

Merged
manel1874 merged 1 commit intomainfrom
shuffle
Sep 4, 2024
Merged

feat: add shuffle feature to array with power of two size#69
manel1874 merged 1 commit intomainfrom
shuffle

Conversation

@manel1874
Copy link
Copy Markdown
Contributor

This PR introduces support for array shuffling for arrays with size as a power of two.

Apart from this it also:

  • isort changed some ordering of the imports in some files.

Tests

One test added to tests/nada-tests. It tests arrays of Rational, SecretRational, PublicInteger and SecretInteger To run them use:

nada test shuffle

Copy link
Copy Markdown
Member

@jcabrero jcabrero left a comment

Choose a reason for hiding this comment

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

Great work. I had never seen an implementation of Benes Networks and yours seems very clear. Two small nits:

  • We could have array.shuffle() as a primitive in array.py. Though it may not match Numpy, I think it prevents users from not finding the available routine.
  • I also think this deserves an example in itself, as it is the basis for many card-games or things users may want to implement.

# Shuffling the arrays
shuffled_a = shuffle(a)
shuffled_b = shuffle(b)
shuffled_c = shuffle(c)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Missing closing: ```

@manel1874
Copy link
Copy Markdown
Contributor Author

We could have array.shuffle() as a primitive in array.py. Though it may not match Numpy, I think it prevents users from not finding the available routine.

Ok, I will add this. Thanks.

@manel1874
Copy link
Copy Markdown
Contributor Author

I also think this deserves an example in itself, as it is the basis for many card-games or things users may want to implement.

What do you mean by an example? I have an example in the function docs but I assume you are referring to more than that. Are you proposing to add an example to the examples folder?

@oceans404
Copy link
Copy Markdown
Contributor

hey @manel1874! I think @jcabrero is asking you to create an example program using shuffling in our nada-by-example repo. Then I can add it to the docs!

here's how: https://github.com/NillionNetwork/nada-by-example?tab=readme-ov-file#add-a-new-example

@jcabrero
Copy link
Copy Markdown
Member

jcabrero commented Sep 4, 2024

Yes, you can copy the example in the docstring both into the examples folder and in nada-by-example. In that way, users have a direct way of knowing the shuffle primitive is there.

Comment on lines +39 to +42
result_method_a = shuffled_method_a - shuffled_method_a
result_method_b = shuffled_method_b - shuffled_method_b
result_method_c = shuffled_method_c - shuffled_method_c
result_method_d = shuffled_method_d - shuffled_method_d
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@manel1874 instead of checking that shuffled_method_a - shuffled_method_a == 0, can we loop over all the elements of the original array and the shuffle version and assert that not everything is equal? (i.e., at least two have been swapped). The equality to zero doesn't guarantee us anything -- the elements may not have been swapped at all, or even we may have completely different elements, both these would pass.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agree! Thanks. So, ideally we would test three properties:

  1. Randomness: we can use your idea here. We test it by showing that at least one element is in a different position. This gives us a bit more confidence than the current method but this test might fail, although with a very low probability: the same configuration can happen with the same probability as any other.
  2. The elements are preserved: agree!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Follow-up PR for reference #72

@manel1874 manel1874 deleted the shuffle branch September 12, 2024 09:26
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.

4 participants