#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: |
Description
This contains the functions prime_to_S_part, is_S_unit, is_S_integral for number field ideals.
Attachments (3)
Change History (13)
Changed 12 years ago by
Attachment: | S_part.patch added |
---|
comment:1 Changed 12 years ago by
Status: | new → needs_review |
---|
comment:2 Changed 12 years ago by
comment:3 follow-up: 4 Changed 12 years ago by
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 Changed 12 years ago by
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
Attachment: | trac_9317_doctest_change.patch added |
---|
comment:6 Changed 12 years ago by
Cc: | John Cremona Radoslav Kirov added; John Cremona removed |
---|
comment:7 Changed 12 years ago by
Cc: | John Cremona added; John Cremona removed |
---|
comment:8 Changed 12 years ago by
Milestone: | sage-feature → sage-4.5 |
---|---|
Reviewers: | → Anna Haensch, Erin Beyerstedt |
Status: | needs_review → positive_review |
We applied the patch, ran the doctests, and everything looks good to us!
comment:9 Changed 12 years ago by
Merged in: | → sage-4.5.2.alpha0 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
comment:10 Changed 12 years ago by
Authors: | Alyson Deines, Radoslav Kirov → Aly Deines, Radoslav Kirov |
---|
Shouldn't these be like
is_s_unit
instead of the capital s?