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

Added a FFTPolynomial file #87

Open
wants to merge 49 commits into
base: main
Choose a base branch
from

Conversation

Chillee
Copy link
Collaborator

@Chillee Chillee commented Apr 27, 2019

This PR is mainly to get a conversation started around how we should be representing our polynomials. Looking around at MIT's implementation and Adamant's implementation, there seem to be 2 primary design choices.

First, how do we represent our polynomials. MIT does it with a typedef vector<num> poly. Adamant does it with a struct. I'm leaning towards doing it with a vector<num>, simply due to having slightly less boilerplate. The primary advantage of doing it in a struct is that we can then define a functions as methods of the struct instead of global properties. For example, I think doing poly.inv().resize(K) is more natural than resize(inv(poly), K).

Second, how do we make our polynomials work across the 3 "fields" we support. Namely, doubles, ints with NTT, and ints with FFTMOD. MIT does this by wrapping int modulo operations in a struct. I think we can do this too (probably with ModularArithmetic.h and whatever API changes are necessary). I think with proper enough encapsulation, we should only need to change out one function, namely the *= operator.

I've started implementing the other operations, but I wanted to submit a PR with the basics so we could settle on a structure.

Edit: closes #60.

@simonlindholm
Copy link
Member

TBH I don't have a strong opinion on this. It would be nice to join this together with the existing Polynomial.h; there's probably some shared code, and it would make the code foot print of the existing code less, which is nice since it's basically never used.

@Chillee
Copy link
Collaborator Author

Chillee commented Apr 27, 2019

I was more thinking that after I finish writing this we get rid of the old Polynomial class entirely. I don't think there's much point for it.

@Chillee
Copy link
Collaborator Author

Chillee commented Apr 27, 2019

There's not much in the old Polynomial class, it's not documented, etc. I made a new file simply so things don't break while I write the new version and that I could submit some smaller PR's instead of a huge 100+line file with like...15 functions.

@simonlindholm
Copy link
Member

That's fair. Not that I would mind a large PR; it gives a nice impression of how we imagine things looking in the end. Also, note that PolyRoots.h depends on Polynomial.h and will need some small changes if it's removed.

@Chillee
Copy link
Collaborator Author

Chillee commented Apr 27, 2019

Ok then, if you insist :^)

@Chillee
Copy link
Collaborator Author

Chillee commented May 3, 2019

It seems to make no impact on performance (perhaps even a negative impact).

@Chillee
Copy link
Collaborator Author

Chillee commented May 4, 2019

I've reorganized the functions into 5 files (with their dependencies by the side (including transitive dependencies). I've tried to balance minimizing number of extraneous dependencies, giving enough space for documentation, and grouping together things by theme.

  1. PolynomialBase
  2. PolynomialMod [1]
  3. PolynomialPow [1]
  4. PolynomialEval [1], [2]
  5. PolynomialInterpolate [1], [2], [3], [4] (wouldn't need [3] other than deriv)

#include "../number-theory/ModularArithmetic.h"
#include "FastFourierTransform.h"
#include "FastFourierTransformMod.h"
// #include "NumberTheoreticTransform.h"

Choose a reason for hiding this comment

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

Nit: NTT will be the common case, if that matters to you?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is currently commented out for a stupid reason - NumberTheoreticTransform.h doesn't compile properly on its own (due to redefinitions of const mod). It is good to know that NTT is the common case though.

if (a.empty()) return {0};
poly b(sz(a) + 1);
b[1] = num(1);
rep(i, 2, sz(b)) b[i] = b[mod%i]*Mod(-mod/i+mod);

Choose a reason for hiding this comment

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

Doesn't work for complex, probably worth noting.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is it even worth noting stuff that doesn't work with complex numbers? Or is it reasonable to assume that every problem involving polynomials/series will be done on a finite field?

Choose a reason for hiding this comment

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

Idk, the field of reals seems like it could be useful sometimes.

Cd z[] = {1, polar(1.0, M_PI / k)};
rep(i, k, 2 * k) rt[i] = Cd(rt[i / 2]) * z[i & 1];
}
}
rep(i,0,n) if (i < rev[i]) swap(a[i], a[rev[i]]);

Choose a reason for hiding this comment

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

You can merge this loop with the generating loop if you want, would keep things more compact and (marginally) improve cache locality.

* Date: 2017-05-10
* License: CC0
* Source: Wikipedia
* Author: chilli, Andrew He, Adamant

Choose a reason for hiding this comment

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

Much of my work comes from THU, probably should give them credit too.

@Chillee
Copy link
Collaborator Author

Chillee commented May 5, 2019

I removed the euclid dependency in ModularArithmetic.h for 2 reasons:

  1. It makes the total Polynomial code longer :^)
  2. I suspect that there's not much point in supporting division on coprime values. I can't think of places where one might want to use ModularArithmetic on a ring that isn't a finite field. If someone really needs to, it's easy enough to change it.

@ecnerwala
Copy link

If you want, here's my modinv:
int modinv(int a, int m) { return a == 1 ? 1 : m - 1ll * modinv(m % a, a) * m / a; }
Requires 1 <= a < m and gcd(a,m) == 1

@ecnerwala
Copy link

Also, empirically, modinv is somewhere between 1x and 2x faster than modular exponentiation.

@Chillee Chillee changed the title Started adding a FFTPolynomial file [WIP] Started adding a FFTPolynomial file May 11, 2019
@Chillee Chillee changed the title Started adding a FFTPolynomial file Added a FFTPolynomial file May 16, 2019
Copy link
Member

@simonlindholm simonlindholm left a comment

Choose a reason for hiding this comment

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

Sorry for the extremely belated review... Would be nice to get this merged.

ll v;
Mod() : v(0) {}
Mod(ll vv) : v(vv % mod) {}
Mod operator+(Mod b) { return Mod((v + b.v) % mod); }
Copy link
Member

Choose a reason for hiding this comment

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

% mod is unnecessary

Mod operator*(Mod b) { return Mod((x * b.x) % mod); }
ll v;
Mod() : v(0) {}
Mod(ll vv) : v(vv % mod) {}
Copy link
Member

Choose a reason for hiding this comment

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

ll vv = 0 to get rid of the default constructor

ll x, y, g = euclid(a.x, mod, x, y);
assert(g == 1); return Mod((x + mod) % mod);
}
Mod invert(Mod a) { return a^(mod-2); }
Copy link
Member

Choose a reason for hiding this comment

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

assumes the modulo is prime, which is probably worth a comment. Also, inline this into operator/. Or maybe keep this method with a comment about it being only for composite moduli.

@@ -0,0 +1,36 @@
/**
Copy link
Member

Choose a reason for hiding this comment

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

(need to merge with master to get rid of these fft changes from the diff)

typedef vector<num> poly;
poly &operator+=(poly &a, const poly &b) {
a.resize(max(sz(a), sz(b)));
rep(i, 0, sz(b)) a[i] = a[i] + b[i];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
rep(i, 0, sz(b)) a[i] = a[i] + b[i];
rep(i, 0, sz(b)) a[i] = a[i] + b[i];
rep(i,0,sz(b)) a[i] = a[i] + b[i];

(same in other places)

mine::poly a;
MIT::poly am;
for (int i = 0; i < sz; i++) {
int val = rand();
Copy link
Member

Choose a reason for hiding this comment

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

is this mod a really large number? if so, it would make sense to test zeroes as well, since they are special. It would also make sense to lower the field size to make zeroes happen more naturally. (and also maybe using some fancier finite field to make sure we don't make too many assumptions about it)


}

const int NUMITERS=10;
Copy link
Member

Choose a reason for hiding this comment

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

way too small. maybe make this non-const and set it in main

template <class A, class B> void testPow(string name, A f1, B f2, int mxSz = 5, int mxPref=5) {
for (int it = 0; it < NUMITERS; it++) {
auto a = genVec((rand() % mxSz) + 1);
int pref = rand()%mxSz;
Copy link
Member

Choose a reason for hiding this comment

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

mxPref?

}
cout<<endl;
}
signed main() {
Copy link
Member

Choose a reason for hiding this comment

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

put this in a special benchmarking main, and have a main that just does testing

auto duration = chrono::duration_cast<chrono::milliseconds>(end - begin).count();
cerr << duration << "ms elapsed [" << label << "]" << endl;
}
};
Copy link
Member

Choose a reason for hiding this comment

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

move this to utilities/bench.h and replace the nonsense I put in there

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.

FFT-based polynomial operations
3 participants