Opened 9 years ago

Closed 8 years ago

#11139 closed defect (fixed)

Ideal with no generators for univariate polynomial rings

Reported by: mraum Owned by: malb
Priority: critical Milestone: sage-4.7.1
Component: commutative algebra Keywords:
Cc: Merged in: sage-4.7.1.alpha1
Authors: Martin Raum, Jeroen Demeyer Reviewers: John Palmieri, Martin Raum
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

Consider the following (Sage 4.7alpha1) :

sage: P.<a> = QQ[]
sage: P.ideal([])
Booom!

The issue is twofold: The ideal method in rings/rings.py is called and does not accept an empty list. But also the univariate polynomial rings should call a specialized method just like the multivariate ones do.

It worked in earlier versions (like 4.2 or so) and is a construction that easily comes up as a corner case.

Apply 11139_alternative.patch.

Attachments (3)

trac-11139-ideals_with_no_generators.patch (463 bytes) - added by mraum 9 years ago.
trac-11139-ideals_with_no_generators.v2.patch (1.3 KB) - added by jhpalmieri 8 years ago.
11139_alternative.patch (4.2 KB) - added by jdemeyer 8 years ago.
Alternative patch, apply only this

Download all attachments as: .zip

Change History (13)

comment:1 Changed 8 years ago by jhpalmieri

  • Authors set to Martin Raum
  • Reviewers set to John Palmieri
  • Status changed from new to needs_review

I'm happy with this. Here's a new patch with a few doctests illustrating that the problem has been fixed.

I'm going to mark this as "needs review"; it wouldn't hurt to have someone else look it over.

Changed 8 years ago by jhpalmieri

comment:2 Changed 8 years ago by jhpalmieri

  • Description modified (diff)

comment:3 Changed 8 years ago by jdemeyer

  • Authors changed from Martin Raum to Martin Raum, Jeroen Demeyer
  • Description modified (diff)

What do you think of this alternative patch? IMHO, it has better logic.

comment:4 follow-up: Changed 8 years ago by mraum

  • Status changed from needs_review to needs_info

I think that indeed the logic is much better. Though it might be confusing to beginners. But this improvement brings my attention to a problem, that has been there before:

R.<x> = QQ[]
R.ideal((x for _ in range(2)))

This is an instance of GeneratorType?, that is called with len. So either we exclude the GeneratorType? (no change to the situtation before) or correct it. Also, mind that a ring element might be an instance of GeneratorType?. In that (very exceptional) situation, the new logic would cause trouble.

Any experience with similar code at other places in Sage?

comment:5 in reply to: ↑ 4 Changed 8 years ago by jdemeyer

  • Status changed from needs_info to needs_review

Replying to mraum:

This is an instance of GeneratorType?, that is called with len. So either we exclude the GeneratorType? (no change to the situtation before) or correct it.

I would say not to check for GeneratorType. I have added a new patch for this. I still have to test it thoroughly.

Changed 8 years ago by jdemeyer

Alternative patch, apply only this

comment:6 Changed 8 years ago by jdemeyer

  • Description modified (diff)

Patch tested, all tests passed.

comment:7 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-4.7 to sage-4.7.1
  • Priority changed from blocker to critical

comment:8 Changed 8 years ago by jdemeyer

  • Description modified (diff)

Any reviewers?

comment:9 Changed 8 years ago by mraum

  • Reviewers changed from John Palmieri to John Palmieri, Martin Raum
  • Status changed from needs_review to positive_review

I think, since I haven't written this ultimate patch, I am a legitimate reviewer. All tests pass and also the patch bot does not complain. Although we have changed the logic, backward compatibility is preserved.

comment:10 Changed 8 years ago by jdemeyer

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