Opened 13 years ago

Closed 12 years ago

#7883 closed enhancement (fixed)

Added some functionality to ideals

Reported by: Robert Bradshaw Owned by: Martin Albrecht
Priority: major Milestone: sage-4.6
Component: commutative algebra Keywords: ideals
Cc: David Roe Merged in: sage-4.6.alpha2
Authors: David Roe Reviewers: Robert Bradshaw, David Loeffler
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by David Loeffler)

Added some functionality to ideals (is_maximal works more often now, and there's a new ideal class for univariate polynomial rings). Changed the logic on what class is chosen for ideals so that it's easier to override with _ideal_class_. Number of generators?

Prerequisite for #8333, #8334, #8335, #9887.

Attachments (4)

7585_7_ideal.patch (20.0 KB) - added by Robert Bradshaw 13 years ago.
7883_ideals.patch (19.7 KB) - added by David Roe 13 years ago.
Rebased against 4.3.3; may need 8218, 8332, 7880, but probably not.
7883_fixes.patch (4.3 KB) - added by David Roe 12 years ago.
Addresses referee comments
trac_7883-ideals-folded.patch (22.4 KB) - added by David Loeffler 12 years ago.
Apply only this patch

Download all attachments as: .zip

Change History (12)

Changed 13 years ago by Robert Bradshaw

Attachment: 7585_7_ideal.patch added

comment:1 Changed 13 years ago by Robert Bradshaw

Description: modified (diff)
Status: newneeds_work

There's a lot of useful looking code in here, but it's unclear what exactly it does or it's intended to fix. Some more explanations would be nice, and the new behavior should be doctested. Especially for most of the changes in sage/rings/ideal.py

What is the parameter in _ideal_class_ supposed to mean? (Then why is it 0 by default?)

Depends on #7881?

Changed 13 years ago by David Roe

Attachment: 7883_ideals.patch added

Rebased against 4.3.3; may need 8218, 8332, 7880, but probably not.

comment:2 Changed 13 years ago by David Roe

I haven't made the changes Robert suggested, but I wanted to get an updated patch up since other finite field functionality depends on this.

comment:3 Changed 13 years ago by David Roe

Part of a series:

8218 -> 8332 -> 7880 -> 7883 -> 8333 -> 8334 -> 8335

I tried to make each of these mostly self contained, with doctests passing after every ticket, but I didn't entirely succeed. If you're reviewing one of these tickets, applying later tickets will hopefully fix doctest failures that you're seeing.

comment:4 Changed 13 years ago by David Loeffler

Tried this under 4.3.4.rc0 with 8218, 8332, 7880 applied. It applies fine but seven doctests fail. Moreover, I second Robert's criticism: it's not clear what all this new code is actually for. (The fact that other finite-field stuff depends on this doesn't change that.) Can we have a new patch with some more docstrings etc?

Changed 12 years ago by David Roe

Attachment: 7883_fixes.patch added

Addresses referee comments

comment:5 Changed 12 years ago by David Roe

Status: needs_workneeds_review

Apply:

7883_ideals.patch
7883_fixes.patch

Probably will not pass doctests unless you also apply the patches on #8333 and #8334.

Changed 12 years ago by David Loeffler

Apply only this patch

comment:6 Changed 12 years ago by David Loeffler

Keywords: ideals added
Reviewers: Robert BradshawRobert Bradshaw, David Loeffler
Status: needs_reviewpositive_review

I've uploaded a patch above which consists of

  • the two patches mentioned in roed's previous comment
  • the hunk from 7585_12_1_fixes.2.patch (at #8334) that concerns sage/rings/ring.pyx
  • a tiny fix to remove the code in ideal_1poly_field that calls residue_field, since the residue fields code isn't going in until a subsequent patch.

I have checked that this applies cleanly to 4.6.alpha1 and passes long doctests.

comment:7 Changed 12 years ago by David Loeffler

Description: modified (diff)

comment:8 Changed 12 years ago by Mitesh Patel

Merged in: sage-4.6.alpha2
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.