Extended Galois field #38

drskalman · 2019-01-04T19:44:10Z

It is my first ever rust code for public consumption so it probably needs lots of improvement :-)

My goal at this stage was to write something fast so I can make the reed-solomon codes work for GF((2^8)^n) and then improve it afterward. So efficiency, etc wasn't on my mind at this stage.

Although it works for any extension degree n, the tests are written for GF((2^8)^2) extended by x^2 + a*x + a^7 (where a is the generator of GF(2^8) field used in this code).

darrenldl · 2019-01-05T04:55:20Z

@drskalman Thanks very much for the work!

Since @rphmeier 's PR was merged into master before your PR, could you sync the branch from master to resolve the conflicts?

Thanks!

drskalman · 2019-01-05T11:14:55Z

Thanks for looking into this. Sure I can. I'm a rust newbie and I wasn't expecting merge without major change. So it was there more for seeking improvement advice rather than merge. But I'll do that. I'll close this and open a new one after rebase. It would be nice if you can add @rphmeier also as reviewer to the new PR.

darrenldl · 2019-01-05T12:11:27Z

I am not sure if I have taken a look close enough to give proper advice, I do have some thoughts on the implementations however. I have not looked closely enough, so please do correct me if I got the wrong impression.

reed-solomon-erasure v1.X.X was following BackBlaze's original Java implementation, which considers things on value per value basis rather than thinking about things as byte slices. This seems to be what you're doing here. This was okay for very early implementation, but the later much faster implementation couldn't reuse any of the code, and required me to rewrite almost everything from scratch in v2.0.0 copying Klaus Post's Go implementation.
- I'm concerned that something similar might happen here - the faster implementation ends up wasting the effort put in the first general implementation. Personally I feel it might be better to start with the faster implementation from the start, but I'm not very certain on that.
The other option I think might work out better would be to follow jerasure team's gf-complete, which is a C implementation that is mature and supports SIMD. They use the 3-clause BSD license (as per their COPYING file), which I think is compatible with MIT (license of this library). So the translated work should still be able to be distributed under MIT license.
- I have not looked into their work closely either, so I am not certain if this is actually a good idea.
I'd rather add the uncheck stuff at the very end of the upgrade, since it can be very difficult to debug.
Testing wise, I would prefer quickcheck style tests to added along with unit tests as new code is added, or at least when the new code is stabilised, but we can think about this much later. The current quickcheck style tests are written using the quickcheck crate, and the names are prefixed with qc_, but I'm happy with proptest crate (or anything similarly robust) as well.

It would be nice if you can add @rphmeier also as reviewer to the new PR.

I can't add people as "reviewer" properly unless I add them as collaborator to this project, at which point I'd rather move this into an organisation project. But I'll mention him in the new PR.

rphmeier · 2019-01-05T15:21:07Z

src/galois_ext.rs

+    }
+
+    pub fn from(original_rep_polynom: &Polynom) -> ExtendedFieldElement {
+


extraneous whitespace -- also this is better as a From implementation

actually, I can see that there is also a From implementation and that it is equivalent to this function except for the fact that one reduces and one doesn't. What is the correct behavior? I would write to reduce to err on the side of caution.

rphmeier · 2019-01-05T15:21:21Z

src/galois_ext.rs

@@ -0,0 +1,92 @@
+use ::poly::Polynom;
+use poly_math::*;


better not to use glob imports

rphmeier · 2019-01-05T15:21:34Z

src/galois_ext.rs

+    pub fn inverse(self: &Self) -> ExtendedFieldElement {
+        if !self.is_zero() {
+            let (gcd, x, _) = self.rep_polynom.egcd(&EXT_POLY!());
+            //we still need to normilze it by divinig by the gcd


quite a lot of typos in this function. it might also be better to document the fact that inverting zero panics

rphmeier · 2019-01-05T15:22:27Z

src/galois_ext.rs

+    }
+
+
+    pub fn is_zero(self: &Self)-> bool {


self: &Self equivalent to &self

rphmeier · 2019-01-05T15:47:50Z

I'm concerned that something similar might happen here - the faster implementation ends up wasting the effort put in the first general implementation

This is a legitimate concern, but I think as long as we abstract everything sufficiently via traits it shouldn't end up having non-local effects.

I'm pushing a few changes:

Make code more idiomatic
Quickcheck tests for polynomial math

rphmeier · 2019-01-05T16:00:27Z

src/macros.rs

+    }
+}
+
+macro_rules! EXT_POLY {


what is this for?

rphmeier · 2019-01-05T16:01:03Z

src/macros.rs

+macro_rules! uncheck_mut {
+    ($array:ident[$index:expr]) => {
+        * if cfg!(feature = "unsafe_indexing") {
+            unsafe {


I would recommend removing the unsafe indexing entirely. rustc is good at eliding bounds checks where possible

rphmeier · 2019-01-05T16:02:12Z

src/poly.rs

+                *x = 0;
+            }
+        } else if new_len < old_len {
+            self.dirty = true;


I don't see anywhere where the dirty flag is getting set to false again. Why not just ignore the bits further than length?

rphmeier · 2019-01-05T16:02:31Z

src/poly.rs

+    }
+
+    #[inline]
+    pub fn set_length(&mut self, new_len: usize) {


what should happen if new_len > POLYNOMIAL_MAX_LENGTH?

rphmeier · 2019-01-05T16:15:19Z

src/poly.rs

+
+    #[inline]
+    pub fn reverse(mut self) -> Self {
+        (*self).reverse();


doesn't this need to take into account length?

coveralls · 2019-01-06T14:17:12Z

Coverage decreased (-0.4%) to 96.355% when pulling cc374ef on paritytech:extended-galois-field into 0d64fd0 on darrenldl:master.

coveralls · 2019-01-06T14:17:12Z

Coverage decreased (-0.4%) to 96.355% when pulling cc374ef on paritytech:extended-galois-field into 0d64fd0 on darrenldl:master.

coveralls · 2019-01-06T14:17:13Z

Coverage decreased (-0.5%) to 96.227% when pulling f6cf735 on paritytech:extended-galois-field into 0d64fd0 on darrenldl:master.

codecov · 2019-01-06T14:25:03Z

Codecov Report

Merging #38 into master will decrease coverage by 0.41%.
The diff coverage is 91.72%.

@@            Coverage Diff             @@
##           master      #38      +/-   ##
==========================================
- Coverage   96.77%   96.35%   -0.42%     
==========================================
  Files           7        8       +1     
  Lines        2479     2771     +292     
==========================================
+ Hits         2399     2670     +271     
- Misses         80      101      +21

Impacted Files	Coverage Δ
src/galois_8.rs	`99.63% <ø> (ø)`	⬆️
src/lib.rs	`94.11% <ø> (ø)`	⬆️
src/tests/mod.rs	`98% <100%> (ø)`	⬆️
src/matrix.rs	`98.73% <100%> (ø)`	⬆️
src/poly.rs	`89.28% <89.28%> (ø)`
src/galois_ext.rs	`96.15% <96.15%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0d64fd0...cc374ef. Read the comment docs.

codecov · 2019-01-06T14:25:03Z

Codecov Report

Merging #38 into master will decrease coverage by 0.9%.
The diff coverage is 77.62%.

@@            Coverage Diff             @@
##           master      #38      +/-   ##
==========================================
- Coverage   96.77%   95.86%   -0.91%     
==========================================
  Files           7        7              
  Lines        2479     2612     +133     
==========================================
+ Hits         2399     2504     +105     
- Misses         80      108      +28

Impacted Files	Coverage Δ
src/galois_8.rs	`99.63% <ø> (ø)`	⬆️
src/lib.rs	`94.11% <ø> (ø)`	⬆️
src/tests/mod.rs	`98% <100%> (ø)`	⬆️
src/matrix.rs	`98.73% <100%> (ø)`	⬆️
src/galois_16.rs	`77.3% <77.3%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0d64fd0...f6cf735. Read the comment docs.

darrenldl · 2019-01-08T06:49:39Z

Just want to confirm something in case I'm being stupid, so the .sage file is only for testing the formulas manually, right?

(Mainly want to ensure no static look up tables are/will be generated by sage/non-Rust code)

rphmeier · 2019-01-08T09:02:34Z

The sage formula is used to generate the irreducible polynomial we use as a modulus for GF(2^16), but not at build time.

In the future we will probably drop the polynomial logic and extend with another table of size 256, generated at build time.

rphmeier · 2019-01-08T09:42:44Z

Ok, I think this should be ready for review now.

I managed to get rid of the arbitrary length polynomials by porting over a couple specialized cases of polynomial division and the extended euclidean algorithm to the galois_16 module.

darrenldl · 2019-01-08T15:44:28Z

Thanks very much @drskalman @rphmeier !

I'll try to get to reviewing it in a day or so, next 3 days are especially busy for me, so apologies if I don't end up reviewing it in time as suggested.

darrenldl · 2019-01-08T16:33:03Z

src/galois_16.rs

+        }
+
+        // either self is zero polynomial or is equivalent to 0
+        panic!("Cannot invert 0");


Personally I feel it would be nicer to move the panic!(...) to top the top of the code, or change this function into if ... else .. branching, as these two options feel easier for audit and analysis.

Just to be more specific, moving it to top would be

if self.zero(...) { panic!("Cannot invert 0"); } // first step of extended euclidean algorithm. ...

changing into if ... else ... branch would be

if self.zero(...) { panic!("Cannot invert 0"); } else { // first step of extended euclidean algorithm. ... }

Largely my habit though, the main rationale would be regardless of function size, the panic!()s would always be visible at top of function, or at the top of one of the branches.

darrenldl · 2019-01-08T16:55:21Z

Seems best to also include tests for Element::inverse specifically. Though div, which is tested, invokes inverse directly, I'm not sure if the test cases invoke inverse sufficiently thoroughly.

I'll come back later to look over the remaining portion.

darrenldl · 2019-01-09T03:02:53Z

The tests look good, I might add more tests later on just to safe guard future changes, but I feel the coverage should be solid enough for now.

Thanks for the work again @drskalman @rphmeier

rphmeier · 2019-01-09T15:29:22Z

More tests are always a bonus!

drskalman · 2019-01-09T19:17:03Z

Just want to confirm something in case I'm being stupid, so the .sage file is only for testing the formulas manually, right?
Right, I left it there in case if we need to generate higher degree irreducible polynomials and tests for them.

(Mainly want to ensure no static look up tables are/will be generated by sage/non-Rust code)
no static look up.

rphmeier reviewed Jan 5, 2019

View reviewed changes

src/galois_ext.rs Outdated

@@ -0,0 +1,92 @@

use ::poly::Polynom;

use poly_math::*;

Copy link

Contributor

rphmeier Jan 5, 2019

Choose a reason for hiding this comment

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

better not to use glob imports

rphmeier reviewed Jan 5, 2019

View reviewed changes

src/galois_ext.rs Outdated

}

pub fn is_zero(self: &Self)-> bool {

Copy link

Contributor

rphmeier Jan 5, 2019

Choose a reason for hiding this comment

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

self: &Self equivalent to &self

rphmeier reviewed Jan 5, 2019

View reviewed changes

src/macros.rs Outdated

}

}

macro_rules! EXT_POLY {

Copy link

Contributor

rphmeier Jan 5, 2019

Choose a reason for hiding this comment

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

what is this for?

rphmeier reviewed Jan 5, 2019

View reviewed changes

rphmeier force-pushed the extended-galois-field branch from c1c98f5 to 2791287 Compare January 5, 2019 21:11

drskalman and others added 9 commits January 6, 2019 15:00

Added struct ExtendedFieldElement for arithmetics over GF((2^8)^n)

24904b0

Added attribution for the polynomial over GF(256) code

85c2c0f

remove uncheck macro

5bc8bd0

move fill_random to tests module

b44a87b

merge poly and poly_math modules

135c4d6

fiddle with Polynom

8431384

alter div test to pass (checking division results)

b374994

all tests pass

0cf93c7

some 2018 changes

cc374ef

rphmeier force-pushed the extended-galois-field branch from 2791287 to cc374ef Compare January 6, 2019 14:02

initial galois_16 implementation

cf3a273

rphmeier added 2 commits January 7, 2019 17:58

attempt to do fancy egcd without polynomial logic

6207f2f

replace ExtendedFieldElement with a [u8; 2] wrapper

6677c18

rphmeier force-pushed the extended-galois-field branch from dec113d to 6677c18 Compare January 7, 2019 17:15

rphmeier added 2 commits January 8, 2019 10:20

quickcheck for polynom

c4e3b4e

do GF((2^8)^16) inversion all in one file

0615363

rphmeier mentioned this pull request Jan 8, 2019

Make generic over a Field trait #40

Merged

darrenldl reviewed Jan 8, 2019

View reviewed changes

restructure inverse and fix exponentiation

f6cf735

darrenldl merged commit 0890891 into rust-rse:master Jan 9, 2019

rphmeier deleted the extended-galois-field branch January 9, 2019 15:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extended Galois field #38

Extended Galois field #38

drskalman commented Jan 4, 2019

darrenldl commented Jan 5, 2019

drskalman commented Jan 5, 2019

darrenldl commented Jan 5, 2019

rphmeier Jan 5, 2019 •

edited

Loading

rphmeier Jan 5, 2019

rphmeier Jan 5, 2019

rphmeier Jan 5, 2019 •

edited

Loading

rphmeier Jan 5, 2019

rphmeier commented Jan 5, 2019 •

edited

Loading

rphmeier Jan 5, 2019

rphmeier Jan 5, 2019

rphmeier Jan 5, 2019

rphmeier Jan 5, 2019 •

edited

Loading

rphmeier Jan 5, 2019

coveralls commented Jan 6, 2019

coveralls commented Jan 6, 2019

coveralls commented Jan 6, 2019 •

edited

Loading

codecov bot commented Jan 6, 2019

codecov bot commented Jan 6, 2019 •

edited

Loading

darrenldl commented Jan 8, 2019 •

edited

Loading

rphmeier commented Jan 8, 2019

rphmeier commented Jan 8, 2019

darrenldl commented Jan 8, 2019

darrenldl Jan 8, 2019 •

edited

Loading

darrenldl commented Jan 8, 2019 •

edited

Loading

darrenldl commented Jan 9, 2019

rphmeier commented Jan 9, 2019

drskalman commented Jan 9, 2019 •

edited

Loading

		}

		pub fn from(original_rep_polynom: &Polynom) -> ExtendedFieldElement {

Extended Galois field #38

Extended Galois field #38

Conversation

drskalman commented Jan 4, 2019

darrenldl commented Jan 5, 2019

drskalman commented Jan 5, 2019

darrenldl commented Jan 5, 2019

rphmeier Jan 5, 2019 • edited Loading

Choose a reason for hiding this comment

rphmeier Jan 5, 2019

Choose a reason for hiding this comment

rphmeier Jan 5, 2019

Choose a reason for hiding this comment

rphmeier Jan 5, 2019 • edited Loading

Choose a reason for hiding this comment

rphmeier Jan 5, 2019

Choose a reason for hiding this comment

rphmeier commented Jan 5, 2019 • edited Loading

rphmeier Jan 5, 2019

Choose a reason for hiding this comment

rphmeier Jan 5, 2019

Choose a reason for hiding this comment

rphmeier Jan 5, 2019

Choose a reason for hiding this comment

rphmeier Jan 5, 2019 • edited Loading

Choose a reason for hiding this comment

rphmeier Jan 5, 2019

Choose a reason for hiding this comment

coveralls commented Jan 6, 2019

coveralls commented Jan 6, 2019

coveralls commented Jan 6, 2019 • edited Loading

codecov bot commented Jan 6, 2019

Codecov Report

codecov bot commented Jan 6, 2019 • edited Loading

Codecov Report

darrenldl commented Jan 8, 2019 • edited Loading

rphmeier commented Jan 8, 2019

rphmeier commented Jan 8, 2019

darrenldl commented Jan 8, 2019

darrenldl Jan 8, 2019 • edited Loading

Choose a reason for hiding this comment

darrenldl commented Jan 8, 2019 • edited Loading

darrenldl commented Jan 9, 2019

rphmeier commented Jan 9, 2019

drskalman commented Jan 9, 2019 • edited Loading

rphmeier Jan 5, 2019 •

edited

Loading

rphmeier Jan 5, 2019 •

edited

Loading

rphmeier commented Jan 5, 2019 •

edited

Loading

rphmeier Jan 5, 2019 •

edited

Loading

coveralls commented Jan 6, 2019 •

edited

Loading

codecov bot commented Jan 6, 2019 •

edited

Loading

darrenldl commented Jan 8, 2019 •

edited

Loading

darrenldl Jan 8, 2019 •

edited

Loading

darrenldl commented Jan 8, 2019 •

edited

Loading

drskalman commented Jan 9, 2019 •

edited

Loading