Skip to content

Add bindings for type qfb_t #282

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

remyoudompheng
Copy link

This is a proposal to add bindings for type qfb_t as a new class qfb.

This implementation defines operators * and ** allowing computation in class groups (similar to the interface of cypari2 type Qfb and SageMath BinaryQF) and a few convenience constructors such as prime_form.

Let me know if a specific convention should be followed for test cases and documentation.

@oscarbenjamin
Copy link
Collaborator

Thanks for this. I am sure that it is good to add this but I will have to take some time to learn what qfb is...

Are there any other group types in FLINT that could in future be exposed in python-flint?

I'm wondering if it makes sense to have any kind of abstract superclass like the ones in src/flint/flint_base/flint_base.pxd. At least it probably makes sense for this to subclass flint_elem.

Would it make sense at all for these to have a context (parent) or is that not really needed?

Most other types are going to use the generic interface in future and will end up having contexts in future sort of like:

F = Zmod(5)
print(F(2) / F(3)

Does the generics part of flint include anything related to qfb?

Apart from the linting job the CI failures are unrelated. I think that they are all caused by a new Cython release. I will fix that.

cdef fmpz_t L
fmpz_abs(L, q1.D.val)
fmpz_root(L, L, 4)
qfb_nucomp(res.val, q1.val, (<qfb>q2).val, q1.D.val, L)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The doc for qfb_nucomp says that q1 and q2 need to have common discriminant.

What happens here if they don't have the same discriminant?

Is it difficult to check?

@oscarbenjamin
Copy link
Collaborator

An .rst file would need to be add in the docs directory referencing this class.

It would good to have a docstring with doctest example for each non-dunder method and a short demonstration in the class docstring that illustrates the basic dunder methods and defines what the class represents. It doesn't need to be long but just clear about what this is with a clear demonstration of usage.

@fredrik-johansson
Copy link
Collaborator

Are there any other group types in FLINT that could in future be exposed in python-flint?

Other than Dirichlet characters (which are already wrapped): permutation groups and the modular group, which both have generics wrappers. There's no qfb generics wrapper yet.

I must admit that I'm not familiar enough with binary quadratic forms to say exactly what the interface here should be, but my rough understanding is that you might want two different types:

  1. A type representing any binary quadratic form, i.e. a qfb_t without further restrictions. (These don't form a nice algebraic structure, as far as I know.)

  2. A type representing an element of a class group with fixed discriminant D by means of a qfb_t. For this, it would make more sense to store D in a context object defining the group rather than adjoining it to the element. You would also presumably want to enforce reducedness (or maybe have an option flag in the context object for whether to allow unreduced representations).

I guess 2) is more convenient for mathematical applications (and would be logical to do via generics), unless you have a specific use case for unreduced forms requiring 1)?

@oscarbenjamin
Copy link
Collaborator

I'm going to reopen to rerun the tests after fixing the cython issue in gh-284.

@oscarbenjamin
Copy link
Collaborator

There are two CI failures. One is the lint issue and the other is that the setuptools build fails. The qfb module needs to be added somewhere in here:

python-flint/setup.py

Lines 77 to 81 in 2ac9797

ext_files = [
("flint.pyflint", ["src/flint/pyflint.pyx"]),
("flint.flint_base.flint_base", ["src/flint/flint_base/flint_base.pyx"]),
("flint.flint_base.flint_context", ["src/flint/flint_base/flint_context.pyx"]),

The meson coverage job shows that some lines are not covered by the tests:

Name                 Stmts   Miss  Cover   Missing
--------------------------------------------------
flint/types/qfb.pyx     80     13    84%   35, 39, 43, 48, 53, 67, 70, 92-97

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.

3 participants