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 )
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.
Attachments (1)
Change History (13)
comment:1 Changed 8 years ago by
- Cc burcin added
comment:2 follow-up: ↓ 8 Changed 8 years ago by
- Cc malb vbraun added
comment:3 Changed 7 years ago by
- Milestone changed from sage-5.3 to sage-5.4
- Priority changed from major to critical
comment:4 Changed 7 years ago by
- Cc SimonKing nbruin added
comment:5 Changed 7 years ago by
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.
comment:6 Changed 6 years ago by
- Description modified (diff)
- Status changed from new to needs_review
comment:7 Changed 6 years ago by
- Description modified (diff)
comment:8 in reply to: ↑ 2 Changed 6 years ago by
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 ofsage/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:9 Changed 6 years ago by
Common pitfall for C++ users, see also https://groups.google.com/d/msg/cython-users/zWxngTspfsY/ItcbwkIqR20J
comment:10 Changed 6 years ago by
- Reviewers set to Volker Braun
- Status changed from needs_review to positive_review
comment:11 Changed 6 years ago by
- Milestone changed from sage-5.11 to sage-5.12
comment:12 Changed 6 years ago by
- Merged in set to sage-5.12.beta1
- Resolution set to fixed
- Status changed from positive_review to closed
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:
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 tostderr
now?In any case,
GroebnerStrategy.__cinit__()
raises an error on line 99 ofsage/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.