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

[WIP] Combined addition and multiplication for fmpz_mpoly #994

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

Conversation

BrentBaccala
Copy link

fmpz_mpoly_addmul_multi and its variants constructs the sum of a list of products of fmpz_mpoly's without storing intermediate results

multiplication; no optimization for small coefficients or small
exponents; no documentation
   1. maxfields wasn't computed correctly
   2. exponent offsets are multiplied by "N"
   3. testing a candidate multiindex to see it it's on an edge
      is more complicated than just checking if it's less than 0
… exponents,

that were commented out anyway because they haven't been ported
Works on existing single-term test cases, but hasn't been tested multi-term yet.
@tthsqe12
Copy link
Contributor

tthsqe12 commented Sep 2, 2021

@wbhart could you approve this for CI?
Code look good, and I can't make sense of the the AppVeyor errors.

@wbhart
Copy link
Collaborator

wbhart commented Oct 12, 2021

The code looks ok so far, however it fails on Windows (take a look at the appveyor CI). I think this is likely due to the function pointers. I'm not sure how to do those on Windows but I assume there's tutorials out there somewhere. Of course I didn't check this, so it could be something else of course.

@wbhart
Copy link
Collaborator

wbhart commented Oct 12, 2021

How do you envision documentation working? It's a very specific application. To make it a permanent part of Flint, we probably need to document it clearly in its own section so that people know what it is for and how to use it (with examples).

@BrentBaccala
Copy link
Author

How do you envision documentation working? It's a very specific application. To make it a permanent part of Flint, we probably need to document it clearly in its own section so that people know what it is for and how to use it (with examples).

That sounds good. I've started work on expanding the documentation in MPolyAlgorithms, but a new section in the main documentation sounds like a good idea.

@BrentBaccala
Copy link
Author

The code looks ok so far, however it fails on Windows (take a look at the appveyor CI). I think this is likely due to the function pointers. I'm not sure how to do those on Windows but I assume there's tutorials out there somewhere. Of course I didn't check this, so it could be something else of course.

I just noticed that we've got appveyor running on this PR. I haven't tried to build on Windows at all; I'll take a look at it.

…d of slong,

which makes more sense and helps silence compiler warnings in SAGE
@BrentBaccala
Copy link
Author

The code looks ok so far, however it fails on Windows (take a look at the appveyor CI). I think this is likely due to the function pointers. I'm not sure how to do those on Windows but I assume there's tutorials out there somewhere. Of course I didn't check this, so it could be something else of course.

I just noticed that we've got appveyor running on this PR. I haven't tried to build on Windows at all; I'll take a look at it.

It looks like it just doesn't include the new source files in the library, and doesn't find the functions when linking the test programs.

I just added the new source files to the fmpz_mpoly directory, and put test files in fmpz_mpoly/test.

Is there anything else that needs to be done for Windows?

@wbhart
Copy link
Collaborator

wbhart commented Oct 15, 2021

Possibly the CMakeLists file needs updating. Not sure.

@albinahlback
Copy link
Collaborator

Are there any profiling programs with this against fmpz_mpoly_add and fmpz_mpoly_mul?

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.

5 participants