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

Mod experiment #75

Merged
merged 3 commits into from Sep 7, 2023
Merged

Mod experiment #75

merged 3 commits into from Sep 7, 2023

Conversation

deinst
Copy link
Contributor

@deinst deinst commented Sep 7, 2023

This commit allows cython access to almost all of the documented interfaces from the currently referenced include files. I skipped functions that need GMP or stdio files, but they should not be hard to add. I commented out functions that reference structs that python flint did not know about (or more accurately structs that I did not know that python flint may have known about).

Warning: There are bugs in the documentation. These bugs are repeated in the .pxd files (the ones I noticed are fixed, but I doubt I noticed all of them). I stopped counting at 5 (and those are the ones egregious enough that I noticed them). I will write some code that compares the signatures in the documentation to the signatures in the headers before I do this again when we change to flint 3.0.

This should be the last asteroid sized commit for a while. God willing.

This mined the rst files for the documentation to get the signatures of the
functions.  Functions using structures not yet implemented in python-flint are
commented out.
Note: The documentation is not always correct.  I'll automatically compare the
documentation with the headers as soon as I write some code to do so.
@GiacomoPope
Copy link
Contributor

Is it worth killing the dead code:

"""
cdef extern from "flint/fmpz_mpoly_factor.h":
    ctypedef struct fmpz_mpoly_factor_struct:
        fmpz_t content
        fmpz_mpoly_struct * poly
        fmpz_struct * exp
        slong length
        slong alloc
    ctypedef fmpz_mpoly_factor_struct fmpz_mpoly_factor_t[1]
    void fmpz_mpoly_factor_init(fmpz_mpoly_factor_t fac, const fmpz_mpoly_ctx_t ctx)
    void fmpz_mpoly_factor_clear(fmpz_mpoly_factor_t fac, const fmpz_mpoly_ctx_t ctx)
    int fmpz_mpoly_factor(fmpz_mpoly_factor_t fac, const fmpz_mpoly_t A, int full, const fmpz_mpoly_ctx_t ctx)
"""

Inside flintlib/flint.pdx ?

@GiacomoPope
Copy link
Contributor

GiacomoPope commented Sep 7, 2023

Also I was looking at adding fmpz_mod, I don't think this has been added into the flintlib? Is this a case of "all flint2.9 functions" being all currently supported types?

My hope via #50 is to eventually have support for $GF(p^k)$, which for large characteristic will need fq and fq_poly which internally need fmpz_mod and fmpz_mod_poly I believe. (The fq_nmod can use nmod and nmod_poly which are (partially) already implemented)

@deinst
Copy link
Contributor Author

deinst commented Sep 7, 2023

Certainly, nuke it. I will be adding a fmpz_mpoly_factor.pxd soon. I'm working on getting fmpz_mpoly, fmpq_mpoly, and fmpz_mpoly_q implemented, but got distracted with the great refactor.

@GiacomoPope
Copy link
Contributor

I can't nuke it myself (I think Oscar can?), but if it's not cleaned up here I'll remove it in my next PR.

I'm in the same position: I wanted to add some types but realised the refactor needed to happen to make it all a more sensible job going forward!

@deinst
Copy link
Contributor Author

deinst commented Sep 7, 2023

I did not add types that were not initially supported by python-flint. For two reasons. First and foremost, I have no even minimal tests for code that is not currently referenced by python-flint. We may never get complete tests of all the functions, but minimal testing is vastly better than none. Second is that we are a few months away from flint 3.0, and although the interface changes are minimal, I do not relish doing this twice.

Ensuring that the current tests pass is not a complete validation, but it is vastly better than nothing. If you want to add fmpz_mod definitely do so. I'm going to add a bunch of mpoly related code.
I can automatically get the function prototypes from the documentation, but the structures takes some human supervision, and everything needs some testing. In theory, we should get the entire public interface into flintlib but it seems best to put this off till we move to flint 3.0, and resolve the packaging problems. Then we can add the flintlib pxd's to the package data so external cython code can use the interface.

@GiacomoPope
Copy link
Contributor

I'm tempted to work on improving what we have currently and adding new types after flint3 -- I have a feeling that the benefits of the generic ring being implemented will reduce code duplication.

In fact, already I think the flint_base classes could have more structure (for example implementing add which calls _add which would have to be written for each type separately for the correct call to the C function)

Flint has a lot of great work and it would be nice to try and limit the boring work in the future of adding new types and functions by being clever now.

@oscarbenjamin
Copy link
Collaborator

Okay, this looks good.

@oscarbenjamin oscarbenjamin merged commit 82ede3a into flintlib:master Sep 7, 2023
16 checks passed
@oscarbenjamin
Copy link
Collaborator

I can automatically get the function prototypes from the documentation, but the structures takes some human supervision

Can you add the script to do this in bin/? It doesn't need to be perfect but it is useful for others to be able to see it and then they could use it to add other types. If the script is in the codebase then it can also be improved or a better version can be added.

Ideally the situation would be that the flint/flintib/*.pxd files are automatically generated by a script from template files looking something like:

# fmpz.pxd.template

cdef extern from "flint/fmpz.h":
    ctypedef fmpz_struct fmpz_t[1]

    # Auto generated stuff goes here:
    {insert-functions}

Then the script would parse the doc files and generate the pxd files. Some exceptional cases (like doc bugs) would need to be hard-coded into the scripts I guess but then the idea would be to maintain the scripts along with the hard-coded exceptions rather than maintaining the .pxd files directly. We might only sometimes run the scripts manually rather than having them run in CI or something but it is still useful that the script is there and that the pxd files are in a state that can be regenerated automatically.

Second is that we are a few months away from flint 3.0, and although the interface changes are minimal, I do not relish doing this twice.

I agree that it makes sense to switch to Flint 3.0 before attempting to complete this.

I'm tempted to work on improving what we have currently and adding new types after flint3

Maybe we should just move to Flint 3 now?

It's already the case that python-flint only works with precise versions of Flint and Arb so anyone who wants to build python-flint needs to get exact versions deliberately to do so. Users will get their Flint bundled in the wheels so it doesn't directly affect them if the wheel contains Flint 3 that is not yet "released".

Longer term I don't see any benefit in supporting Flint 2.9 either: Flint 3 can be the minimum supported version. I realise that Flint 3 is only at alpha stage but realistically much of python-flint is still more like pre-release quality in general relative to Flint's standards. At this stage in development it is better to focus on the easiest path forwards for the long term future so maybe just moving to Flint 3 right now does that. I don't think that we should hold up development efforts now just to wait for Flint 3 to be released.

I have a feeling that the benefits of the generic ring being implemented will reduce code duplication

I'm not sure but I don't think that the generic rings will replace all the benefit of implementing all of Flint's types in much the same way that is already done in python-flint. Much of the important code to be wrapped is not really generic.

In fact, already I think the flint_base classes could have more structure (for example implementing add which calls _add which would have to be written for each type separately for the correct call to the C function)

Things like this would be good but we don't need to perfect the existing code before adding new features. We can just add new types and build out what looks like a good structure in those new types. Then later if we are happy with the structure some of the older code could be changed to the new structure if there is a reason to make changes. (If the code works then there might be no good reason to change it.)

@GiacomoPope
Copy link
Contributor

I guess the only reason not to go to flint3 now is if prerelease it still has some bugs that would make python-flint unstable? I've not got much experience with flint3 so I couldn't say.

Agree about writing a script for the flintlib pyx -- very nice idea

@oscarbenjamin
Copy link
Collaborator

I guess the only reason not to go to flint3 now is if prerelease it still has some bugs that would make python-flint unstable?

I expect that python-flint has (or will have) enough of its own bugs or inconsistencies that using flint3's alpha releases will not be a dominant factor in its stability in the short term.

@deinst
Copy link
Contributor Author

deinst commented Sep 8, 2023

I agree.
I'll clean up the generation code and submit it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants