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

doctest failure in hilbert_poincare_series with singular 4.2.1 #33134

Closed
tornaria opened this issue Jan 9, 2022 · 39 comments
Closed

doctest failure in hilbert_poincare_series with singular 4.2.1 #33134

tornaria opened this issue Jan 9, 2022 · 39 comments

Comments

@tornaria
Copy link
Contributor

tornaria commented Jan 9, 2022

This was found on 9.5-rc0, using singular 4.2.1 from system (void linux, we haven't upgraded to 4.2.1p3 yet). In case this is relevant, flint is up to date (2.8.4). I wasn't able to reproduce on 9.5-beta9.

sage -t --long --random-seed=136266901849903582203467517333467427001 src/sage/rings/polynomial/hilbert.pyx
**********************************************************************
File "src/sage/rings/polynomial/hilbert.pyx", line 581, in sage.rings.polynomial.hilbert.hilbert_poincare_series
Failed example:
    J.hilbert_numerator(algorithm='singular')
Exception raised:
    Traceback (most recent call last):
      File "/usr/lib/sage-9.5.rc0/local/var/lib/sage/venv-python3.10/lib/python3.10/site-packages/sage/doctest/forker.py", line 694, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/usr/lib/sage-9.5.rc0/local/var/lib/sage/venv-python3.10/lib/python3.10/site-packages/sage/doctest/forker.py", line 1088, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.rings.polynomial.hilbert.hilbert_poincare_series[11]>", line 1, in <module>
        J.hilbert_numerator(algorithm='singular')
      File "/usr/lib/sage-9.5.rc0/local/var/lib/sage/venv-python3.10/lib/python3.10/site-packages/sage/rings/polynomial/multi_polynomial_ideal.py", line 297, in __call__
        return self.f(self._instance, *args, **kwds)
      File "/usr/lib/sage-9.5.rc0/local/var/lib/sage/venv-python3.10/lib/python3.10/site-packages/sage/rings/qqbar_decorators.py", line 96, in wrapper
        return func(*args, **kwds)
      File "/usr/lib/sage-9.5.rc0/local/var/lib/sage/venv-python3.10/lib/python3.10/site-packages/sage/rings/polynomial/multi_polynomial_ideal.py", line 2900, in hilbert_numerator
        hs = hilb(gb, 1, attributes={gb: {'isSB': 1}})
      File "sage/libs/singular/function.pyx", line 1334, in sage.libs.singular.function.SingularFunction.__call__ (build/cythonized/sage/libs/singular/function.cpp:15222)
        return call_function(self, args, ring, interruptible, attributes)
      File "sage/libs/singular/function.pyx", line 1532, in sage.libs.singular.function.call_function (build/cythonized/sage/libs/singular/function.cpp:17224)
        raise RuntimeError("error in Singular function call %r:\n%s" %
    RuntimeError: error in Singular function call 'hilb':
    int overflow in hilb 1
**********************************************************************
1 item had failures:
   1 of  13 in sage.rings.polynomial.hilbert.hilbert_poincare_series
    [25 tests, 1 failure, 0.22 s]
----------------------------------------------------------------------

Depends on #33160

CC: @dkwo @antonio-rojas @orlitzky @simon-king-jena

Component: build: configure

Branch/Commit: u/dimpase/ticket/33134 @ 83af1a8

Reviewer: Dima Pasechnik

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

@tornaria tornaria added this to the sage-9.5 milestone Jan 9, 2022
@tornaria
Copy link
Contributor Author

tornaria commented Jan 9, 2022

comment:1

This doctest was changed in 720d10e, so I guess (a) this is not random at all (b) we need to update singular in void linux.

@tornaria tornaria changed the title random error in hilbert_poincare_series doctest failure in hilbert_poincare_series with singular 4.2.1 Jan 9, 2022
@tornaria
Copy link
Contributor Author

tornaria commented Jan 9, 2022

comment:2

@antonio-rojas: I wonder if we should change/remove this doctest, or else require singular >= 4.2.1p2?

Edit: I meant 4.2.1p2

@mkoeppe
Copy link
Contributor

mkoeppe commented Jan 9, 2022

comment:4

See also #32789

@antonio-rojas
Copy link
Contributor

comment:5

Replying to @tornaria:

@antonio-rojas: I wonder if we should change/remove this doctest, or else require singular >= 4.2.1p2?

Edit: I meant 4.2.1p2

That's what I already asked in #32907 comment:18

@orlitzky
Copy link
Contributor

orlitzky commented Jan 9, 2022

comment:6

Is this the only failing test with 4.2.1? Either way, I think we should change the tests to pass with >=singular-4.2.1, for two reasons. First, v4.2.1 is not that old, and it's polite to give distros some time to catch up. Second, testing for "patchlevel 2 or higher" is a pain in the ass because it doesn't show up in the pkg-config file.

@orlitzky
Copy link
Contributor

orlitzky commented Jan 9, 2022

comment:7

It's going to take me a while to build rc0, but something like this should do the trick:

    This example exceeded the capabilities of Singular before version 4.2.1p2:: 
                                                                                
        sage: expected = (120*t^33 - 3465*t^32 + 48180*t^31 - 429374*t^30       
        ....:             + 2753520*t^29 - 13522410*t^28 + 52832780*t^27        
        ....:             - 168384150*t^26 + 445188744*t^25 - 987193350*t^24    
        ....:             + 1847488500*t^23 + 1372406746*t^22 - 403422496*t^21  
        ....:             - 8403314*t^20 - 471656596*t^19 + 1806623746*t^18     
        ....:             + 752776200*t^17 + 752776200*t^16 - 1580830020*t^15   
        ....:             + 1673936550*t^14 - 1294246800*t^13 + 786893250*t^12  
        ....:             - 382391100*t^11 + 146679390*t^10 - 42299400*t^9      
        ....:             + 7837830*t^8 - 172260*t^7 - 468930*t^6 + 183744*t^5  
        ....:             - 39270*t^4 + 5060*t^3 - 330*t^2 + 1)                 
        ....: actual = None                                                     
        sage: try:                                                              
        ....:     actual = J.hilbert_numerator(algorithm='singular')            
        ....: except RuntimeError:                                              
        ....:     pass                                                          
        ....: actual is None or (actual == expected)                            
        True                                                                    

@tornaria
Copy link
Contributor Author

comment:8

Replying to @orlitzky:

Is this the only failing test with 4.2.1? Either way, I think we should change the tests to pass with >=singular-4.2.1, for two reasons. First, v4.2.1 is not that old, and it's polite to give distros some time to catch up. Second, testing for "patchlevel 2 or higher" is a pain in the ass because it doesn't show up in the pkg-config file.

Yes, this is the only failing test for me.

I'm not sure what's the purpose of this example/test. It seems to me the simplest thing is to tag it as "# not tested - requires singular >=4.2.1p2".

@orlitzky
Copy link
Contributor

comment:9

Well, something else is going on. With 4.2.1p3...

sage: n=4; m=11; P = PolynomialRing(QQ,n*m,"x")
sage: x = P.gens(); M = Matrix(n,x)
sage: I = P.ideal(M.minors(2))
sage: J = P*[m.lm() for m in I.groebner_basis()]
sage: expected = J.hilbert_numerator()
sage: actual = J.hilbert_numerator(algorithm='singular')
sage: actual == expected
False
sage: actual - expected
4294967296*t^22 - 4294967296*t^21 + 4294967296*t^20 - 4294967296*t^19 + 4294967296*t^18

@orlitzky
Copy link
Contributor

comment:10

Simon, we should be expecting the Hilbert numerators to be the same with both algorithm=sage and algorithm=singular, right?

@tornaria
Copy link
Contributor Author

comment:11

We updated to singular-4.3.0 (released wed) and everything passes now.

@orlitzky
Copy link
Contributor

comment:12

Replying to @tornaria:

We updated to singular-4.3.0 (released wed) and everything passes now.

Do the "sage" and "singular" algorithms still produce different results (comment:9)? At this point I'm more worried that we've added a doctest for the wrong answer...

@tornaria
Copy link
Contributor Author

comment:13

Replying to @orlitzky:

Replying to @tornaria:

We updated to singular-4.3.0 (released wed) and everything passes now.

Do the "sage" and "singular" algorithms still produce different results (comment:9)? At this point I'm more worried that we've added a doctest for the wrong answer...

I'm getting the same as you. I couldn't help but notice that the difference is a multiple of 2^32. In fact the coefficients we get from singular are just the truncation to int32_t of the ones we get from sage.

May be a bug in singular?

@tornaria
Copy link
Contributor Author

comment:14

.. or in the conversion from singular -> sage?

@orlitzky
Copy link
Contributor

Author: Michael Orlitzky

@orlitzky
Copy link
Contributor

Branch: u/mjo/ticket/33134

@orlitzky
Copy link
Contributor

comment:15

I've opened #33178 to investigate the underlying problem since that will take more time. For now let's just get rid of the test.


New commits:

7326adcTrac #33134: remove a failing doctest in sage.rings.polynomial.hilbert.

@orlitzky
Copy link
Contributor

Commit: 7326adc

@slel
Copy link
Member

slel commented Jan 30, 2022

comment:16

Set milestone to sage-9.6 after Sage 9.5 release.

@slel slel modified the milestones: sage-9.5, sage-9.6 Jan 30, 2022
@tscrim
Copy link
Collaborator

tscrim commented Apr 7, 2022

comment:17

Wouldn't it be better to mark it as # known bug?

@orlitzky
Copy link
Contributor

orlitzky commented Apr 7, 2022

comment:18

Replying to @tscrim:

Wouldn't it be better to mark it as # known bug?

While it is a known bug, I don't see what benefit there is to (temporarily) recording one of the many possible wrong answers in the sage source code. Once the bug is fixed in #33178, the test will be brought back with the correct answer and a meaningful explanation.

@dimpase
Copy link
Member

dimpase commented Jul 11, 2022

Reviewer: Dima Pasechnik

@dimpase
Copy link
Member

dimpase commented Jul 11, 2022

comment:24

this just adds one #not tested example, the rest is taken care in #33160

@vbraun
Copy link
Member

vbraun commented Jul 17, 2022

comment:25

On 32-bit:

**********************************************************************
File "src/sage/rings/polynomial/hilbert.pyx", line 583, in sage.rings.polynomial.hilbert.hilbert_poincare_series
Failed example:
    J.hilbert_numerator(algorithm='singular')
Expected nothing
Got:
    overflow at t^22
    overflow at t^21
    overflow at t^20
    overflow at t^19
    overflow at t^18
    120*t^33 - 3465*t^32 + 48180*t^31 - 429374*t^30 + 2753520*t^29 - 13522410*t^28 + 52832780*t^27 - 168384150*t^26 + 445188744*t^25 - 987193350*t^24 + 1847488500*t^23 + 752776200*t^17 + 752776200*t^16 - 1580830020*t^15 + 1673936550*t^14 - 1294246800*t^13 + 786893250*t^12 - 382391100*t^11 + 146679390*t^10 - 42299400*t^9 + 7837830*t^8 - 172260*t^7 - 468930*t^6 + 183744*t^5 - 39270*t^4 + 5060*t^3 - 330*t^2 + 1
**********************************************************************

@dimpase
Copy link
Member

dimpase commented Jul 17, 2022

comment:27

one needs to add 32-bit test case

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 17, 2022

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

83af1a8add 32-bit catch-all case

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 17, 2022

Changed commit from 9b8e7cb to 83af1a8

@dimpase
Copy link
Member

dimpase commented Jul 17, 2022

comment:29

this should be it

@dimpase
Copy link
Member

dimpase commented Jul 17, 2022

Dependencies: #33160

@vbraun vbraun modified the milestones: sage-9.7, sage-9.8 Jul 18, 2022
@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 18, 2022

Changed author from Michael Orlitzky to none

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 18, 2022

comment:32

I've made it part of #33160 now

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 18, 2022

Changed reviewer from Dima Pasechnik to none

@mkoeppe mkoeppe removed this from the sage-9.8 milestone Jul 18, 2022
@dimpase
Copy link
Member

dimpase commented Jul 18, 2022

Reviewer: Dima Pasechnik

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

8 participants