#9317 closed enhancement (fixed)
prime_to_S_part, is_S_unit, is_S_integral
| Reported by: | aly.deines | Owned by: | davidloeffler |
|---|---|---|---|
| Priority: | minor | Milestone: | sage-4.5.2 |
| Component: | number fields | Keywords: | S_part |
| Cc: | cremona, rkirov | 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 9 years ago by
comment:1 Changed 9 years ago by
- Status changed from new to needs_review
comment:2 Changed 9 years ago by
comment:3 follow-up: ↓ 4 Changed 9 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 in reply to: ↑ 3 Changed 9 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 9 years ago by
comment:5 Changed 9 years ago by
I found and fixed error in the doctest.
comment:6 Changed 9 years ago by
- Cc John Cremona rkirov added; John Cremona removed
comment:7 Changed 9 years ago by
- Cc cremona added; John Cremona removed
comment:8 Changed 9 years ago by
- Milestone changed from sage-feature to sage-4.5
- Reviewers set to Anna Haensch, Erin Beyerstedt
- Status changed from needs_review to positive_review
We applied the patch, ran the doctests, and everything looks good to us!
comment:9 Changed 9 years ago by
- Merged in set to sage-4.5.2.alpha0
- Resolution set to fixed
- Status changed from positive_review to closed
Shouldn't these be like
is_s_unitinstead of the capital s?