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

Added some functionality to ideals #7883

Closed
robertwb opened this issue Jan 9, 2010 · 14 comments
Closed

Added some functionality to ideals #7883

robertwb opened this issue Jan 9, 2010 · 14 comments

Comments

@robertwb
Copy link
Contributor

robertwb commented Jan 9, 2010

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.

CC: @roed314

Component: commutative algebra

Keywords: ideals

Author: David Roe

Reviewer: Robert Bradshaw, David Loeffler

Merged: sage-4.6.alpha2

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

@robertwb

This comment has been minimized.

@robertwb
Copy link
Contributor Author

robertwb commented Jan 9, 2010

comment:1

Attachment: 7585_7_ideal.patch.gz

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?

@roed314
Copy link
Contributor

roed314 commented Feb 23, 2010

Attachment: 7883_ideals.patch.gz

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

@roed314
Copy link
Contributor

roed314 commented Feb 23, 2010

comment:2

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.

@roed314
Copy link
Contributor

roed314 commented Feb 23, 2010

comment:3

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.

@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Mar 17, 2010

comment:4

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?

@roed314
Copy link
Contributor

roed314 commented Sep 19, 2010

Attachment: 7883_fixes.patch.gz

Addresses referee comments

@roed314
Copy link
Contributor

roed314 commented Sep 19, 2010

comment:5

Apply:

7883_ideals.patch
7883_fixes.patch

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

@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Sep 23, 2010

Apply only this patch

@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Sep 23, 2010

comment:6

Attachment: trac_7883-ideals-folded.patch.gz

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 Improvements to residue fields #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.

@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Sep 23, 2010

Changed keywords from none to ideals

@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Sep 23, 2010

Changed reviewer from Robert Bradshaw to Robert Bradshaw, David Loeffler

@loefflerd

This comment has been minimized.

@qed777
Copy link
Mannequin

qed777 mannequin commented Sep 28, 2010

Merged: sage-4.6.alpha2

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

3 participants