feat(math): implement binary field initially #416

chokobole · 2024-05-12T15:34:49Z

Description

This PR implements BinaryField for binius.

See https://gitlab.com/IrreducibleOSS/binius

tachyon/math/finite_fields/binary_prime_field.h

tachyon/math/finite_fields/binary_fields/binary_fields.h

tachyon/math/finite_fields/binary_prime_field.h

TomTaehoonKim

LGTM

tachyon/math/finite_fields/generator/prime_field_generator/binary_prime_field_config.h.tpl

tachyon/math/finite_fields/binary_prime_field.h

tachyon/math/finite_fields/binary_fields/binary_fields.h

tachyon/math/finite_fields/binary_prime_field.h

tachyon/math/finite_fields/binary_tower_operations.h

tachyon/math/finite_fields/binary_prime_field.h

tachyon/math/finite_fields/generator/prime_field_generator/binary_prime_field128_config.h.tpl

docs/how_to_contribute/math/how_to_add_prime_fields.md

dongchangYoo

LGTM

tachyon/math/base/big_int.h

Insun35

LGTM

ashjeong

Please change the following commit messages!
e1f8e85
feat(math): implement BinaryField initially up to 8 bits
Also change for a4e9f33
390c5f5
Remove the "the"
feat(base): add (u)int128 cases to base::Range

ashjeong · 2024-05-14T09:31:37Z

tachyon/math/finite_fields/binary_fields/BUILD.bazel

        ":binary_field4",
+        ":binary_field64",
        ":binary_field8",


Could you remove clang formatting on this to they can be in order of 1,2,4,8,16,32,64?

ashjeong · 2024-05-30T06:38:12Z

tachyon/math/finite_fields/prime_field_generator_unittest.cc

-  EXPECT_EQ(f * f_inv, PrimeField::One());
-  f.InverseInPlace();
-  EXPECT_EQ(f, f_inv);
+  if (!f.IsZero()) {


I've changed this in #424 check this commit 92e338d
NOTE: it is set to change currently as per reviews I've received, but regardless, will be changed!

ashjeong · 2024-05-31T06:34:06Z

tachyon/math/finite_fields/generator/prime_field_generator/build_defs.bzl

@@ -180,7 +201,10 @@ def _do_generate_prime_fields(
        use_asm,
        use_montgomery,
        **kwargs):
-    is_small_prime_field = int(modulus) < 1 << 32
+    modulus_int = int(modulus)
+    is_binary_prime_field = modulus_int == 1 << 1 or modulus_int == 1 << 2 or modulus_int == 1 << 4 or modulus_int == 1 << 8 or \


All of the moduli beside the first if statement do not denote a prime field! Please change binary field to be not prime field (except maybe F2)

ashjeong · 2024-05-31T07:06:37Z

tachyon/math/finite_fields/binary_tower_operations.h

+    SubPrimeField z2 = lhs1 * rhs1;
+    SubPrimeField z0z2 = z0 + z2;
+    SubPrimeField z1 = (lhs0 + lhs1) * (rhs0 + rhs1) - z0z2;
+    SubPrimeField z2a = BinaryTowerOperations<SubPrimeField>::MulByAlpha(z2);
+    return F::Compose(z0z2, z1 + z2a);


can you rename these to be in order?

Suggested change

SubPrimeField z2 = lhs1 * rhs1;

SubPrimeField z0z2 = z0 + z2;

SubPrimeField z1 = (lhs0 + lhs1) * (rhs0 + rhs1) - z0z2;

SubPrimeField z2a = BinaryTowerOperations<SubPrimeField>::MulByAlpha(z2);

return F::Compose(z0z2, z1 + z2a);

SubPrimeField z1 = lhs1 * rhs1;

SubPrimeField z0z1 = z0 + z1;

SubPrimeField z2 = (lhs0 + lhs1) * (rhs0 + rhs1) - z0z1;

SubPrimeField z1a = BinaryTowerOperations<SubPrimeField>::MulByAlpha(z1);

return F::Compose(z0z1, z2 + z1a);

Actually I've taken the naming convention from the original code, though. Okay!

ashjeong · 2024-05-31T07:08:05Z

tachyon/math/finite_fields/binary_tower_operations.h

+    SubPrimeField z2 = x1.Square();
+    SubPrimeField z2a = BinaryTowerOperations<SubPrimeField>::MulByAlpha(z2);
+    return F::Compose(z0 + z2, z2a);


Suggested change

SubPrimeField z2 = x1.Square();

SubPrimeField z2a = BinaryTowerOperations<SubPrimeField>::MulByAlpha(z2);

return F::Compose(z0 + z2, z2a);

SubPrimeField z1 = x1.Square();

SubPrimeField z1a = BinaryTowerOperations<SubPrimeField>::MulByAlpha(z1);

return F::Compose(z0 + z1, z1a);

ashjeong · 2024-05-31T07:58:26Z

tachyon/math/base/big_int.h

+  constexpr BigInt operator&(const BigInt& other) const {
+    BigInt ret;
+    if constexpr (N == 1) {
+      ret[0] = limbs[0] & other[0];
+    } else if constexpr (N == 2) {
+      ret[0] = limbs[0] & other.limbs[0];
+      ret[1] = limbs[1] & other.limbs[1];
+    } else {
+      DoAnd(*this, other, ret);


Is the main desire for these logic statement hopes of returning the simple cases early? I feel as though simply having DoAnd(*this, other, ret); only would suffice otherwise

Yes, it is!

ashjeong · 2024-05-31T08:04:07Z

tachyon/math/finite_fields/generator/prime_field_generator/build_defs.bzl

-    is_small_prime_field = int(modulus) < 1 << 32
+    modulus_int = int(modulus)
+    is_binary_prime_field = modulus_int == 1 << 1 or modulus_int == 1 << 2 or modulus_int == 1 << 4 or modulus_int == 1 << 8 or \
+                            modulus_int == 1 << 16 or modulus_int == 1 << 32 or modulus_int == 1 << 64 or modulus_int == 1 << 128


I think this line should be spliced up into the commits of 64/128!

ashjeong · 2024-05-31T08:05:58Z

tachyon/math/finite_fields/generator/generator_util.cc

@@ -14,6 +14,15 @@

 namespace tachyon::math {

+bool IsBinaryField(const mpz_class& m) {
+  mpz_class binary_modulus(2);
+  for (int i = 0; i < 8; ++i) {


Though nitpicky, I think changing this to i < 4, 7, 8 for the commits of up to 8, 64, 128 would help to prevent confusion.

ashjeong · 2024-05-31T08:09:31Z

tachyon/math/finite_fields/binary_prime_field.h

+      std::conditional_t<kBits == 16, uint16_t,
+                         std::conditional_t<kBits == 32, uint32_t, uint64_t>>>;


Please add this to the next up to 64 commit!

ashjeong · 2024-05-31T08:10:13Z

tachyon/math/finite_fields/generator/prime_field_generator/prime_field_generator.cc

+      case 16 + 1:
+        modulus_type = "uint32_t";
+        value_type = "uint16_t";
+        break;
+      case 32 + 1:
+        modulus_type = "uint64_t";
+        value_type = "uint32_t";
+        break;


I think these should be added to the next commit!

See https://gitlab.com/IrreducibleOSS/binius/-/blob/main/crates/field/src/binary_field.rs?ref_type=85b1e2ab76f1f981484825f8893c9cdeb38ae5b1 and https://gitlab.com/IrreducibleOSS/binius/-/blob/main/crates/field/src/binary_field_arithmetic.rs?ref_type=85b1e2ab76f1f981484825f8893c9cdeb38ae5b1.

…ction

See https://gitlab.com/IrreducibleOSS/binius/-/blob/main/crates/field/src/binary_field.rs?ref_type=85b1e2ab76f1f981484825f8893c9cdeb38ae5b1 and https://gitlab.com/IrreducibleOSS/binius/-/blob/main/crates/field/src/binary_field_arithmetic.rs?ref_type=85b1e2ab76f1f981484825f8893c9cdeb38ae5b1.

…educexx` are input together

chokobole requested review from dongchangYoo, Insun35, TomTaehoonKim and ashjeong May 12, 2024 15:34

TomTaehoonKim requested changes May 16, 2024

View reviewed changes

TomTaehoonKim reviewed May 16, 2024

View reviewed changes

tachyon/math/finite_fields/binary_prime_field.h Outdated Show resolved Hide resolved

chokobole force-pushed the feat/implement-binary-field-initially branch from 34f7b89 to 4f9f771 Compare May 16, 2024 04:56

TomTaehoonKim approved these changes May 16, 2024

View reviewed changes

chokobole marked this pull request as draft May 16, 2024 11:21

chokobole force-pushed the feat/implement-binary-field-initially branch from 4f9f771 to 4c63976 Compare May 23, 2024 08:38

test(math): skip inverse operation test when value is zero

07e0bb7

chokobole force-pushed the feat/implement-binary-field-initially branch from 4c63976 to 285cb26 Compare May 28, 2024 06:12

chokobole marked this pull request as ready for review May 28, 2024 06:13

chokobole requested a review from TomTaehoonKim May 28, 2024 08:29

dongchangYoo requested changes May 29, 2024

View reviewed changes

chokobole force-pushed the feat/implement-binary-field-initially branch from 285cb26 to 24dd738 Compare May 29, 2024 13:27

dongchangYoo approved these changes May 30, 2024

View reviewed changes

Insun35 requested changes May 30, 2024

View reviewed changes

tachyon/math/base/big_int.h Outdated Show resolved Hide resolved

tachyon/math/base/big_int.h Outdated Show resolved Hide resolved

Insun35 reviewed May 30, 2024

View reviewed changes

tachyon/math/base/big_int.h Outdated Show resolved Hide resolved

chokobole force-pushed the feat/implement-binary-field-initially branch from 24dd738 to 6a1026c Compare May 30, 2024 07:44

Insun35 approved these changes May 31, 2024

View reviewed changes

ashjeong requested changes May 31, 2024

View reviewed changes

chokobole added 8 commits May 31, 2024 21:23

refac(base): use public absl::SimpleHexAtoi instead of internal fun…

68bbb38

…ction

feat(base): add (u)int128 cases to base::Range

6a41233

docs: capitalize elliptic curves

c3a7ddf

style: format how_to_add_prime_fields.md

1365d4d

build(math): stop generating prime field when use_montgomery and `r…

6b7fea3

…educexx` are input together

chokobole added 2 commits May 31, 2024 21:34

docs: add the descriptions of the missing attributes

aec3dce

docs: update how_to_add_prime_fields.md

332a181

chokobole force-pushed the feat/implement-binary-field-initially branch from 6a1026c to 332a181 Compare May 31, 2024 12:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(math): implement binary field initially #416

feat(math): implement binary field initially #416

chokobole commented May 12, 2024

TomTaehoonKim left a comment

dongchangYoo left a comment

Insun35 left a comment

ashjeong left a comment

ashjeong May 14, 2024

ashjeong May 30, 2024

ashjeong May 31, 2024

ashjeong May 31, 2024

chokobole May 31, 2024

ashjeong May 31, 2024

ashjeong May 31, 2024

chokobole May 31, 2024

ashjeong May 31, 2024

ashjeong May 31, 2024 •

edited

ashjeong May 31, 2024

ashjeong May 31, 2024

		std::conditional_t<kBits == 16, uint16_t,
		std::conditional_t<kBits == 32, uint32_t, uint64_t>>>;

feat(math): implement binary field initially #416

Are you sure you want to change the base?

feat(math): implement binary field initially #416

Conversation

chokobole commented May 12, 2024

Description

TomTaehoonKim left a comment

Choose a reason for hiding this comment

dongchangYoo left a comment

Choose a reason for hiding this comment

Insun35 left a comment

Choose a reason for hiding this comment

ashjeong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashjeong May 31, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashjeong May 31, 2024 •

edited