Opened 8 years ago

Closed 6 years ago

#12188 closed defect (fixed)

Bug in is_smooth for curves over CC

Reported by: johanbosman Owned by: AlexGhitza
Priority: critical Milestone: sage-5.12
Component: algebraic geometry Keywords: singular
Cc: burcin, malb, vbraun, SimonKing, nbruin Merged in: sage-5.12.beta1
Authors: Peter Bruin Reviewers: Volker Braun
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by pbruin)

sage: P.<X,Y,Z> = CC[]
sage: C = Curve(X)
sage: C.is_smooth()
singular_ring_delete(ring*) called with NULL pointer.
...
Exception KeyError: (The ring pointer 0x0,) in 'sage.libs.singular.ring.singular_ring_delete' ignored
True

The cause is that if GroebnerStrategy.__cinit__() raises an exception, its __dealloc__ method is still called. Therefore __dealloc__() should only delete things that were actually constructed before the exception occurred.

Ticket #14902 is a duplicate of this one.

Apply: trac_12188-singular_null_pointer.patch

Attachments (1)

trac_12188-singular_null_pointer.patch (749 bytes) - added by pbruin 6 years ago.
check for NULL pointer before deleting Singular ring

Download all attachments as: .zip

Change History (13)

comment:1 Changed 8 years ago by johanbosman

  • Cc burcin added

comment:2 follow-up: Changed 8 years ago by burcin

  • Cc malb vbraun added

I'm not sure if this is actually supposed to work in Sage right now. Multivariate polynomial rings over CC use a generic implementation. The error message should just say "First parameter's ring must be multivariate polynomial ring via libsingular." This is not entirely clear, but at least it is a less scary message than "singular_ring_delete(ring*) called with NULL pointer."

Here is a shorter segment to reproduce the problem:

sage: P.<X,Y,Z> = CC[]
sage: I = P*X
sage: GroebnerStrategy(I)
  File "/home/burcin/sage/sage-4.7.2/local/bin/sage-ipython", line 52, in <module>
    ipy_sage.mainloop(sys_exit=1, banner='')
  File "/home/burcin/sage/sage-4.7.2/local/lib/python2.6/site-packages/IPython/Shell.py", line 76, in mainloop
    self.IP.mainloop(banner)
  File "/home/burcin/sage/sage-4.7.2/local/lib/python2.6/site-packages/IPython/iplib.py", line 1762, in mainloop
    self.interact(banner)
  File "/home/burcin/sage/sage-4.7.2/local/lib/python2.6/site-packages/IPython/iplib.py", line 2001, in interact
    more = self.push(line)
  File "/home/burcin/sage/sage-4.7.2/local/lib/python2.6/site-packages/IPython/iplib.py", line 2305, in push
    more = self.runsource('\n'.join(self.buffer), self.filename)
  File "/home/burcin/sage/sage-4.7.2/local/lib/python2.6/site-packages/IPython/iplib.py", line 2231, in runsource
    if self.runcode(code) == 0:
  File "/home/burcin/sage/sage-4.7.2/local/lib/python2.6/site-packages/IPython/iplib.py", line 2260, in runcode
    exec code_obj in self.user_global_ns, self.user_ns
  File "<ipython console>", line 1, in <module>
Exception KeyError: (The ring pointer 0x0,) in  ignored
singular_ring_delete(ring*) called with NULL pointer.
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)

/home/burcin/sage/sage-4.7.2/<ipython console> in <module>()

/home/burcin/sage/sage-4.7.2/local/lib/python2.6/site-packages/sage/libs/singular/groebner_strategy.so in sage.libs.singular.groebner_strategy.GroebnerStrategy.__cinit__ (sage/libs/singular/groebner_strategy.cpp:2169)()

TypeError: First parameter's ring must be multivariate polynomial ring via libsingular.

This is the message I get after adding an "except +" after the declaration of singular_ring_delete().

It seems that GroebnerStrategy.__cinit__() (why __cinit__ and not __init__?) already has a doctest for this case. I don't see why the error message is not caught by the doctesting framework. Are these cython errors printed to stderr now?

In any case, GroebnerStrategy.__cinit__() raises an error on line 99 of sage/libs/singular/groebner_strategy.pyx. I thought the __dealloc__() method is not called if there is an error during __init__(). Any ideas what is going wrong here?

I suggest to open a new ticket to use libsingular from multivariate polynomial rings over CC and RR. This ticket can be about eliminating the scary singular_ring_delete error message.

comment:3 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.3 to sage-5.4
  • Priority changed from major to critical

comment:4 Changed 7 years ago by jdemeyer

  • Cc SimonKing nbruin added

comment:5 Changed 7 years ago by nbruin

A quick test reveals that:

P.<X,Y,Z> = CC[]
C = Curve(X)
self = C
d = self.codimension()
minors = self.Jacobian_matrix().minors(d)
I = self.defining_ideal()
minors = tuple([ I.reduce(m) for m in minors ])

(Who ever thought that "Jacobian" was a good name for the singular subscheme? I think most algebraic geometers expect something else when they ask for the Jacobian of a curve).

and a little look indeed shows that this code tries to use "singular"'s reduce (the ideal gets reconstructed several times), and I think it does so successfully (does singular now allow rings with float coefficients?), so it looks indeed like a mismatch in ring deletions. It may be a good test case, because the ring really gets constructed internally, via a _singular_ method.

Changed 6 years ago by pbruin

check for NULL pointer before deleting Singular ring

comment:6 Changed 6 years ago by pbruin

  • Authors set to Peter Bruin
  • Description modified (diff)
  • Status changed from new to needs_review

comment:7 Changed 6 years ago by pbruin

  • Description modified (diff)

comment:8 in reply to: ↑ 2 Changed 6 years ago by pbruin

Replying to burcin:

I'm not sure if this is actually supposed to work in Sage right now. Multivariate polynomial rings over CC use a generic implementation. The error message should just say "First parameter's ring must be multivariate polynomial ring via libsingular." This is not entirely clear, but at least it is a less scary message than "singular_ring_delete(ring*) called with NULL pointer."

With the patch, the example in the ticket description gives the right answer (is_smooth() -> True) and no error.

Here is a shorter segment to reproduce the problem: [...]

Here the correct TypeError message is displayed.

It seems that GroebnerStrategy.__cinit__() (why __cinit__ and not __init__?)

It would be cleaner to separate the initialisation of Python and C attributes into __init__ and __cinit__. I tried changing __cinit__ to __init__ and it did not make any difference.

already has a doctest for this case. I don't see why the error message is not caught by the doctesting framework. Are these cython errors printed to stderr now?

This is indeed surprising.

In any case, GroebnerStrategy.__cinit__() raises an error on line 99 of sage/libs/singular/groebner_strategy.pyx. I thought the __dealloc__() method is not called if there is an error during __init__(). Any ideas what is going wrong here?

Apparently __dealloc__ is called; I didn't check the Cython documentation.

comment:10 Changed 6 years ago by vbraun

  • Reviewers set to Volker Braun
  • Status changed from needs_review to positive_review

comment:11 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:12 Changed 6 years ago by jdemeyer

  • Merged in set to sage-5.12.beta1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.