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

Use libsingular for polynomial rings over function fields #16567

Closed
simon-king-jena opened this issue Jun 27, 2014 · 66 comments
Closed

Use libsingular for polynomial rings over function fields #16567

simon-king-jena opened this issue Jun 27, 2014 · 66 comments

Comments

@simon-king-jena
Copy link
Member

Singular has polynomial rings over function fields. However, Sage makes no use of it:

sage: P.<a> = QQ[]
sage: F = P.fraction_field()
sage: type(F['x','y'])
<class 'sage.rings.polynomial.multi_polynomial_ring.MPolynomialRing_polydict_domain_with_category'>
sage: F = FunctionField(QQ,'a')
sage: type(F['x','y'])
<class 'sage.rings.polynomial.multi_polynomial_ring.MPolynomialRing_polydict_domain_with_category'>

Update:

This works now:

sage: F = PolynomialRing(QQ,'a').fraction_field()
sage: R.<x,y> = F[]
sage: F.inject_variables()
Defining a
sage: I = R.ideal([a*x+5*y^2, (1+a)/(1-a)*x^3-3*y*x])
sage: I.groebner_basis()
[x^3 + (3*a - 3)/(a + 1)*x*y, y^2 + (a)/5*x]

}}}

CC: @malb @miguelmarco @simon-king-jena @dimpase @sagetrac-sbulygin @bhutz @Torrencem @tscrim @jdemeyer @jpflori @embray @EnderWannabe

Component: commutative algebra

Keywords: function field, libsingular

Author: Miguel Marco

Branch/Commit: 6ce2bed

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/16567

@simon-king-jena
Copy link
Member Author

Changed keywords from none to function field, libsingular

@simon-king-jena

This comment has been minimized.

@simon-king-jena
Copy link
Member Author

comment:2

It is not a big deal to create the MPolynomialRing_libsingular with parameters. However, I need to find out how to create a Singular number with parameters. Martin, can you give me a pointer to a relevant file of the Singular sources?

@malb
Copy link
Member

malb commented Jun 28, 2014

comment:3

Looks like you want kernel/longtrans.h:

/*
* ABSTRACT:   numbers in transcendental field extensions,
              i.e., in rational function fields
*/

@simon-king-jena
Copy link
Member Author

comment:4

Replying to @malb:

Looks like you want kernel/longtrans.h:

/*
* ABSTRACT:   numbers in transcendental field extensions,
              i.e., in rational function fields
*/

Thank you, Martin!

So far, I only found number (*nPar)(int i). If I understand correctly, it returns the i-th parameter as a number. But this would probably be cumbersome to use for transforming an element of a Sage function field resp. of a Sage quotient field of polynomial ring.

@simon-king-jena
Copy link
Member Author

comment:5

Seems like one actually needs to construct a number by starting with nPar(i) (which is a function pointer to ntPar, IIUC) and conversion of integers, and then add, multiply and divide. Or is there a way to directly convert a polynomial in variables a,b,c into a number in parameters a,b,c? After all, there is this globally accessible ring nacRing hosting numerator and denominator of a function field element.

@malb
Copy link
Member

malb commented Jun 28, 2014

comment:6

I'd assume there should be a way to convert a polynomial to a number as these numbers are polynomials internally as far as I know. Maybe ask on libsingular-devel?

@simon-king-jena
Copy link
Member Author

@simon-king-jena
Copy link
Member Author

comment:8

Miguel asked me to push the basic steps that I did for this ticket.

All what works is to set up polynomial rings over "fraction fields of polynomial rings" as libsingular polynomial rings with parameters. What is missing is everything else. In particular: the element constructor must be modified so that elements of quotient fields of polynomial rings are converted into libsingular numbers.


New commits:

d15c0ccUse libsingular for polynomial ring with parameters.

@simon-king-jena
Copy link
Member Author

Commit: d15c0cc

@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.3, sage-6.4 Aug 10, 2014
@miguelmarco
Copy link
Contributor

comment:11

Ping!

I am trying (once again) to work on this. I also got to the point of creating the ring, and am stuck in the point of translating the coefficients.

I have started with conversion from sage coefficients to singular ones (the sa2si function in singular.pyx; but for some reason i don't understand, the generated .c code does not import the definitions from transext.h.

@miguelmarco
Copy link
Contributor

comment:12

Also, i am not sure if i am dealing with the pointers correctly. It could use some review freom someone with more c/cython experience.

@nbruin
Copy link
Contributor

nbruin commented Oct 4, 2021

@nbruin
Copy link
Contributor

nbruin commented Oct 4, 2021

comment:13

It's a little easier if the branch you're talking about is actually attached.

The cython docs for "cimport extern" is here:

https://cython.readthedocs.io/en/latest/src/userguide/external_C_code.html#referencing-c-header-files

It actually does say there that the extern declaration should let cython generate an #include "singular/polys/ext_fields/transext.h". However, cython with take it on blind fate that the cython declarations given in the block itself somehow correspond to something that makes sense in C, because cython will not generate any declarations to define the types you specify: the "extern" promises that this is somehow resolved (by the given include or something else). So you should probably check that the generated C file indeed has an #include included. Otherwise, if you made a subtle typo compared with what's in C, the error would only arise in the C compiler, so you should probably carefully read the error that the C compiler generates and check that your spelling of everything matches up. Hope this helps?

@nbruin
Copy link
Contributor

nbruin commented Oct 4, 2021

Changed commit from d15c0cc to 6978419

@miguelmarco
Copy link
Contributor

comment:14

Thanks for the answer!

I did check that: the generated .cpp file has this line

#include "singular/polys/ext_fields/transext.h"

and the file SAGE_LOCAL/local/include/singular/polys/ext_fields/transext.h has the following declarations:

struct fractionObject
{
  poly numerator;
  poly denominator;
  int complexity;
};

typedef struct fractionObject * fraction;

So I still have no idea about what could be going on.

One possible guess is that the generated file is a .cpp, that is, no pure C, but C++. Maybe some problem with scopes?

@miguelmarco
Copy link
Contributor

comment:15

I think I finally got it. This branch seems to work.

Still needs some polishing (documentation, cleanup, and check that we are handling memory correctly... which means lots of testing).

So please be welcome to take a look and stress-test it.

@miguelmarco miguelmarco modified the milestones: sage-6.4, sage-9.5 Oct 6, 2021
@miguelmarco
Copy link
Contributor

Changed commit from 6978419 to 136a89a

@miguelmarco

This comment has been minimized.

@miguelmarco
Copy link
Contributor

New commits:

3de10e6Hacky solution for sa2si (coefficients that fit in int size only)
2d89757Use mpz to create coefficients
64f4f57Working implementation. Still needs cleanup, documentation, and checking for memory leaks
3d3419fFixed case with just one parameter
136a89aFix problem with several parameters introduced by previous fix

@miguelmarco
Copy link
Contributor

comment:36

Linter complains about an unused variable in a method for matrix groups, which is rewriten to use this interface in #19391.

Anyways, in the current implementation, that assignation is necesary to create the corresponding singular ring, even if the python variable is not used later. The linter is just not smart enough in this case.

@tscrim
Copy link
Collaborator

tscrim commented Nov 6, 2021

comment:37

There are some failures reported by the patchbot:

sage -t --long --random-seed=65607106580737169923545380617831998347 src/sage/dynamics/arithmetic_dynamics/projective_ds.py  # 1 doctest failed
sage -t --long --random-seed=65607106580737169923545380617831998347 src/sage/combinat/root_system/non_symmetric_macdonald_polynomials.py  # 1 doctest failed
sage -t --long --random-seed=65607106580737169923545380617831998347 src/sage/interfaces/expect.py  # 2 doctests failed

@miguelmarco
Copy link
Contributor

comment:38

I can't reproduce the error in pexpect or projective_ds.py (I suspect somehow the file i am testing is different from the one in the patchbot).

About the error in macdonald polynomials, I found that it is due to one function creating polynomials in x0, x1... and the other on x1, x2 ... Will look into it.

@miguelmarco
Copy link
Contributor

comment:39

By rebasing to the newest develop version, I can reproduce the error in src/sage/dynamics/arithmetic_dynamics/projective_ds.py. It has to do with code added in #31994.

I am trying to debug it, but it is quite complicated. Adding the people involved in that ticket to cc for help.

@EnderWannabe
Copy link
Contributor

comment:40

I've traced the error to the affine_patch function in src/sage/schemes/projective/projective_subscheme.py. The error is caused by code equivalent to the following

sage: T.<c,d> = QQ[]
sage: F = FractionField(T)
sage: R.<x,y,z> = F[]
sage: Q = F['x', 'z']
sage: f = R(d*z^2 + c*y*z^2)
sage: f((Q.gens()[0], 1, Q.gens()[1]))
TypeError

My usual workaround here is to create a dictionary and attempt f.subs(), however,

f.subs({x: Q.gens()[0], y: 1, z:Q.gens()[1]})

crashes Sage.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 7, 2021

Changed commit from 9424a24 to d412c77

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 7, 2021

Branch pushed to git repo; I updated commit sha1. New commits:

1a94d13Merge branch 'develop' into plural
2704119Fix creating polynomial from strings in the case of field with several generators
84a04d1Fix variable indices in doctest
d412c77Fix crash in .subs()

@miguelmarco
Copy link
Contributor

comment:42

Thanks for the hint. I could pinpoint the origin of both problems. This should solve it.

I still can't reproduce the error in expect.py. Can somebody confirm it happens?

@miguelmarco
Copy link
Contributor

comment:44

One of the patchbot passes all tests, and complains only abut the unused variable (that is actually needed to prevent gargage collection). The other patchbots have what seem to be unrelated problems.

Is it ok to merge this in this state? or should I do some dummy operation with the variable to make pyflakes happy?

@tscrim
Copy link
Collaborator

tscrim commented Nov 12, 2021

comment:45

Then it is a spurious error. I wouldn't worry about the pyflakes error. Thank you.

@vbraun
Copy link
Member

vbraun commented Nov 13, 2021

comment:46

On 32-bit:

**********************************************************************
File "src/sage/rings/polynomial/polydict.pyx", line 1628, in sage.rings.polynomial.polydict.ETuple.eadd
Failed example:
    y^(2^32)
Expected:
    Traceback (most recent call last):
    ...
    OverflowError: exponent overflow (...)
Got:
    <BLANKLINE>
    Traceback (most recent call last):
      File "/var/lib/buildbot/slave/sage_git/build/local/var/lib/sage/venv-python3.9.7/lib/python3.9/site-packages/sage/doctest/forker.py", line 694, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/var/lib/buildbot/slave/sage_git/build/local/var/lib/sage/venv-python3.9.7/lib/python3.9/site-packages/sage/doctest/forker.py", line 1088, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.rings.polynomial.polydict.ETuple.eadd[6]>", line 1, in <module>
        y**(Integer(2)**Integer(32))
      File "sage/rings/polynomial/multi_polynomial_libsingular.pyx", line 2467, in sage.rings.polynomial.multi_polynomial_libsingular.MPolynomial_libsingular.__pow__ (build/cythonized/sage/rings/polynomial/multi_polynomial_libsingular.cpp:23166)
        singular_polynomial_pow(&_p, self._poly, exp, _ring)
    OverflowError: Python int too large to convert to C unsigned long
**********************************************************************
1 item had failures:
   1 of   8 in sage.rings.polynomial.polydict.ETuple.eadd
    [276 tests, 1 failure, 0.26 s]
----------------------------------------------------------------------
sage -t --long --random-seed=307417638528231788267817062701073757912 src/sage/rings/polynomial/polydict.pyx  # 1 doctest failed
----------------------------------------------------------------------

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 13, 2021

Branch pushed to git repo; I updated commit sha1. New commits:

6ce2bedAdapt overflow doctest to 32 bits

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 13, 2021

Changed commit from d412c77 to 6ce2bed

@tscrim
Copy link
Collaborator

tscrim commented Nov 14, 2021

comment:48

LGTM.

@vbraun
Copy link
Member

vbraun commented Nov 15, 2021

@vbraun vbraun closed this as completed in aa66d52 Nov 15, 2021
vbraun pushed a commit to vbraun/sage that referenced this issue Nov 2, 2023
    
minor cleanup, also trying to fix sagemath#14886 (already fixed in sagemath#16567)

### 📝 Checklist

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.
    
URL: sagemath#36593
Reported by: Frédéric Chapoton
Reviewer(s): Kwankyu Lee
vbraun pushed a commit to vbraun/sage that referenced this issue Nov 5, 2023
    
minor cleanup, also trying to fix sagemath#14886 (already fixed in sagemath#16567)

### 📝 Checklist

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.
    
URL: sagemath#36593
Reported by: Frédéric Chapoton
Reviewer(s): Kwankyu Lee
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants