Add fq_default_poly
#182
Add fq_default_poly
#182
Conversation
|
When you get to writing the tests I think it would be good to try to achieve as high as possible coverage through the generic tests. It is fine to add test cases like: if domain is nmod or fq:
assert p.some_func() == p2
else:
# update this when some_func is added to fmpz_poly etc.
assert not hasattr(p, "some_func")It is very useful for future additions if the test cases are generic in so far as possible because it means that we can apply the same tests to all types to check consistency. It also makes it a lot easier to add new types or new methods to types if most of the test cases are already in place. A test that the type does not have the method serves as a reminder to future contributors to update the test when the method is added as well as a clear cue in the code that there is something that is yet to be implemented. |
|
Yeah this is a nice idea and should definitely be done for fmpz_mod, nmod and fq as well as the same as before but _poly. |
|
I haven't made "unique" tests for the type yet, but it's now tested in the generic poly tests. this uncovered a segfault in flint: flintlib/flint#2046 so the polynomial evaluation for certain a certain type is currently an ugly hack |
|
I think aside from reviewer comments, the main thing to do now is run coverage and start making sure I capture as much as possible. |
|
Generally looks good to me. |
|
I'll try and write some detailed tests for everything tomorrow, the coverage for this and fq_default is maybe a little low at the time of writing. |
GiacomoPope
force-pushed
the
add_fq_poly
branch
from
e7e99a8 to
63ce496
Compare
|
latest push uses the |
Rather than copying the test function it would be better if these are handled using generic test code that is shared. Especially if I wrote the Then you added Now The best thing to do here is to try to achieve maximum coverage in generic tests. Then we can use those same tests originally written for e.g. |
|
I would say that first try to get maximum coverage using only generic tests like the At some point there won't be anything left that can reasonably be covered by generic tests but there will likely still be a few things that should be tested specifically for |
Let me check that. I had a working coverage measurement before... |
|
Note that it needs to be: First confirm that flint is not importable: Then with PYTHONPATH set the coverage script should measure coverage: $ PYTHONPATH=src bin/coverage.sh
running build_ext
Running tests...
test_pyflint...OK
...
flint.test: all 52 tests passed!
flint.test: all 3601 doctests passed!
----------------------------------------
OK: Your installation of python-flint seems to be working just fine!
----------------------------------------
Wrote HTML report to htmlcov/index.html
$ coverage report --sort=cover
Name Stmts Miss Cover
------------------------------------------------------------
src/flint/types/fmpz.pxd 2 2 0%
src/flint/types/fq_default.pxd 2 2 0%
src/flint/types/acb_series.pyx 582 573 2%
src/flint/types/arb_series.pyx 576 496 14%
src/flint/types/arb_poly.pyx 217 183 16%
src/flint/types/acb_poly.pyx 257 194 25%
src/flint/types/fmpz_mpoly.pxd 11 8 27%
src/flint/types/arf.pyx 107 76 29%
src/flint/types/fmpq_vec.pyx 47 26 45%
src/flint/flint_base/flint_context.pxd 8 4 50%
src/flint/types/dirichlet.pyx 57 25 56%
src/flint/types/fmpq_mpoly.pxd 11 4 64%
src/flint/types/arb_mat.pyx 313 111 65%
src/flint/types/fmpz_vec.pyx 45 15 67%
src/flint/types/acb_mat.pyx 375 113 70%
src/flint/test/__main__.py 82 18 78%
src/flint/flint_base/flint_base.pyx 222 45 80%
src/flint/types/arb.pyx 830 134 84%
src/flint/types/acb.pyx 900 140 84%
src/flint/functions/showgood.pyx 61 8 87%
src/flint/types/fq_default.pyx 301 39 87%
src/flint/types/fmpz_series.pyx 189 13 93%
src/flint/types/fmpq_series.pyx 363 14 96%
src/flint/flint_base/flint_context.pyx 27 1 96%
src/flint/types/nmod_poly.pyx 335 11 97%
src/flint/types/fmpq_mpoly.pyx 461 11 98%
src/flint/types/fmpq_poly.pyx 272 5 98%
src/flint/types/fmpq.pyx 229 4 98%
src/flint/types/fmpq_mat.pyx 247 4 98%
src/flint/types/nmod_mat.pyx 253 4 98%
src/flint/types/fmpz_mpoly.pyx 446 7 98%
src/flint/types/fmpz_poly.pyx 327 4 99%
src/flint/types/fmpz_mat.pyx 273 3 99%
src/flint/types/fmpz_mod_poly.pyx 595 5 99%
src/flint/test/test_all.py 2983 7 99%
src/flint/types/fmpz.pyx 486 1 99%
src/flint/__init__.py 34 0 100%
src/flint/flint_base/__init__.py 0 0 100%
src/flint/functions/__init__.py 0 0 100%
src/flint/pyflint.pyx 3 0 100%
src/flint/test/__init__.py 0 0 100%
src/flint/types/__init__.py 0 0 100%
src/flint/types/fmpz_mod.pyx 210 0 100%
src/flint/types/fmpz_mod_mat.pyx 287 0 100%
src/flint/types/nmod.pyx 135 0 100%
src/flint/types/nmod_series.pyx 1 0 100%
src/flint/utils/__init__.py 0 0 100%
src/flint/utils/flint_exceptions.py 6 0 100%
------------------------------------------------------------
TOTAL 13168 2310 82%You can see that it is also happening in CI after gh-181: |
|
OK so setting the path might have been the issue, but I'm getting weird compilation errors now... I can comment out these two lines, but further along I then get Which is super super weird. I'll have to try and debug later |
|
This is possibly a problem with mixing setuptools and the in-place build vs editable install vs spin... That's why I said to confirm that flint is not importable: you don't want coverage to pick up the wrong version of the module. You might want to use a separate git checkout for the setuptools coverage build. Otherwise I would recommend using a full |
|
Actually I think that you maybe have an old version of Flint installed in |
|
yeah im not sure, i tried a few things and got different errors. ill have to come back to this another day |
|
I downloaded the branch clean and rand everything which compiled, but setting the python path to src breaks my imports. clang -bundle -undefined dynamic_lookup -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX14.sdk -L/usr/local/opt/openssl@1.1/lib -I/usr/local/include -L/usr/local/lib -I/usr/local/opt/openssl@1.1/include build/temp.macosx-14.0-x86_64-cpython-312/src/flint/functions/showgood.o -lflint -o /Users/Jack/Downloads/python-flint-add_fq_poly/src/flint/functions/showgood.cpython-312-darwin.so
ld: warning: search path '/Users/Jack/Downloads/python-flint-add_fq_poly/.local/lib' not found
No module named 'flint.pyflint'Here you can see i can import it, just things break when i run the bin/coverage with the python path set to source. If I set it to PWD I again only do coverage of the test stuff... |
|
You haven't said where you have installed Flint so I am assuming it is a system-wide install (i.e. not in I'm assuming that can be ignored.
I think that one thing that is confusing things here is that you also have a separate installation of python-flint that is not just the code in the source directory i.e. it is in your virtual environment somewhere or something like that. To avoid confusion you should If you don't set PYTHONPATH then it should not be possible to import flint at all: This error message is expected: The flint package is not executable because it has no |
|
Also if you can import |
|
I dunno why, but it just doesnt behave as yours does. Then the coverage at the end of this call (which actually does something) is the one which shows the test coverage |
|
I don't understand why PYTHONPATH=. works. That shouldn't work unless you have a directory called
I can think of one reason why you are only seeing coverage of .py files rather than Cython files though. You need to set this: Lines 60 to 63 in 5445684 Without that the Cython-generated C code will not output any tracing events that coverage can use to measure the coverage. That is set by the coverage script: Lines 32 to 37 in 5445684 However note the commented out line to "force a rebuild". When you run the coverage script no rebuilding is happening as you showed above: There is a problem with Cython's setuptools build that it does not properly detect when a rebuild is needed in the way that meson does. With meson if you do: Then meson will detect if this requires rebuilding everything or not. Setuptools and Cython's cythonize are not so clever though. That is why the You need to build something like: Then something like this hopefully works: |
|
Just to show paths, this is the current set up: I will try everything you say above though :) |
|
I ran: Which rebuilt everything, as expected. Then If I try and point to the src Unsurprisingly Does the same as no PATH set :) |
|
You have another installation of python-flint here which is being picked up: You possibly installed that with e.g. To avoid confusion uninstall that python-flint until you have things working. You can also run coverage like: That makes sure to run coverage from the environment that corresponds to your The import failure when you set PYTHONPATH=src is because it is a different version of Python and the extension modules are specific to the version: You are building the extensions with Python 3.12 or something but then running coverage from a separate Python 3.11 installation. |
|
OMG I'm so stupid. Thank you for noticing this. My stupid mac has python3.12 under Jack: python-flint-add_fq_poly % python3 -m coverage run -m flint.test
No module named 'flint'
Jack: python-flint-add_fq_poly % PYTHONPATH=src python3 -m coverage run -m flint.test
Running tests...
test_pyflint...OK
test_showgood...OK
test_fmpz...OK
test_fmpz_factor...OKLooks good! I have normal life things to do, but I will work on tests and coverage as the next commit to this repo. thanks for all your help |
|
This is another good reason to get spin coverage working. I'll have another look at it. |
|
From the CI: Which means the targets for this PR seem to have high coverage. Happy for this to go through any last reviews now :) |
src/flint/types/fq_default_poly.pyx
Outdated
| ``True``, ensures that the output is irreducible. | ||
|
|
||
| TODO: there is currently no fq_defualt_poly method for testing that | ||
| a polynomial is irreducible?! |
There was a problem hiding this comment.
We can add one here.
Apart from shortcutting some simple cases the general way to check is just to factorise the polynomial.
There was a problem hiding this comment.
Oh this is outdated there is one it was just in the poly_factor header file rather than the poly header so I missed it when I was writing this.
| n_max = fq_default_poly_deflation( | ||
| self.val, self.ctx.field.val | ||
| ) | ||
| if n > n_max: | ||
| raise ValueError(f"Cannot deflate with {n = }, maximum allowed value is {n_max = }") |
There was a problem hiding this comment.
Is this check necessary to prevent a core dump or something?
There was a problem hiding this comment.
I think so, I had to do it for fmpz_mod and I just assumed the same check would need to be here (It will definitely need to be for the FMPZ_MOD type I suppose if nothing else...)
|
I went through and it looks good. I just had one comment and another question. |
|
I realised that even though inverse mod is missing from FLINT we can just implement it with xgcd, so I did that quickly before bed |
|
Okay, looks good. Thanks again! |
Still a draft, spent this evening just getting the flintlib started and creating the necessary structs