Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#9317 closed enhancement (fixed)

prime_to_S_part, is_S_unit, is_S_integral

Reported by: Alyson Deines Owned by: David Loeffler
Priority: minor Milestone: sage-4.5.2
Component: number fields Keywords: S_part
Cc: John Cremona, Radoslav Kirov Merged in: sage-4.5.2.alpha0
Authors: Aly Deines, Radoslav Kirov Reviewers: Anna Haensch, Erin Beyerstedt
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

This contains the functions prime_to_S_part, is_S_unit, is_S_integral for number field ideals.

Attachments (3)

S_part.patch (3.6 KB) - added by Alyson Deines 12 years ago.
trac_9317-review.patch (4.2 KB) - added by John Cremona 12 years ago.
Apply after previous patch
trac_9317_doctest_change.patch (713 bytes) - added by Alyson Deines 12 years ago.

Download all attachments as: .zip

Change History (13)

Changed 12 years ago by Alyson Deines

Attachment: S_part.patch added

comment:1 Changed 12 years ago by Robert Bradshaw

Status: newneeds_review

comment:2 Changed 12 years ago by Mike Hansen

Shouldn't these be like is_s_unit instead of the capital s?

Changed 12 years ago by John Cremona

Attachment: trac_9317-review.patch added

Apply after previous patch

comment:3 Changed 12 years ago by John Cremona

The code looks good and applies to 4.4.4.alpha1. There were several glitches in the formatting of the docstrings, which I have fixed in the second patch (which needs to be applied after the first).

Note: to test the syntax of the docstrings, do "sage -docbuild reference html" which should rebuild the reference manual pages for files which have been modified. Look out for Warnings and Errors. Also, look at the html output to see if it looks right!

Reply to mhansen: The capital S is standard mathematical notation. This also matches the functions sage.rings.rational.Rational.is_S_integral and sage.rings.rational.Rational.is_S_unit (which I wrote so this is not an independent test!)

Note: this patch was written by some of the students at Sage Days 22, and the intention is that some other students in the same group will review it on Wednesday June 23 as part of their Sage training!

comment:4 in reply to:  3 Changed 12 years ago by Mike Hansen

Replying to cremona:

Reply to mhansen: The capital S is standard mathematical notation. This also matches the functions sage.rings.rational.Rational.is_S_integral and sage.rings.rational.Rational.is_S_unit (which I wrote so this is not an independent test!)

On the other hand, we don't capitalize things like Eulerian in is_eulerian. Whatever is decided, it should be consistent :-)

Changed 12 years ago by Alyson Deines

comment:5 Changed 12 years ago by Alyson Deines

I found and fixed error in the doctest.

comment:6 Changed 12 years ago by Radoslav Kirov

Cc: John Cremona Radoslav Kirov added; John Cremona removed

comment:7 Changed 12 years ago by Radoslav Kirov

Cc: John Cremona added; John Cremona removed

comment:8 Changed 12 years ago by Anna Haensch

Milestone: sage-featuresage-4.5
Reviewers: Anna Haensch, Erin Beyerstedt
Status: needs_reviewpositive_review

We applied the patch, ran the doctests, and everything looks good to us!

comment:9 Changed 12 years ago by Mitesh Patel

Merged in: sage-4.5.2.alpha0
Resolution: fixed
Status: positive_reviewclosed

comment:10 Changed 12 years ago by Alyson Deines

Authors: Alyson Deines, Radoslav KirovAly Deines, Radoslav Kirov
Note: See TracTickets for help on using tickets.