Opened 9 years ago

Closed 9 years ago

Last modified 8 years ago

#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)

S_part.patch (3.6 KB) - added by aly.deines 9 years ago.
trac_9317-review.patch (4.2 KB) - added by cremona 9 years ago.
Apply after previous patch
trac_9317_doctest_change.patch (713 bytes) - added by aly.deines 9 years ago.

Download all attachments as: .zip

Change History (13)

Changed 9 years ago by aly.deines

comment:1 Changed 9 years ago by robertwb

  • Status changed from new to needs_review

comment:2 Changed 9 years ago by mhansen

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

Changed 9 years ago by cremona

Apply after previous patch

comment:3 follow-up: Changed 9 years ago by 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 9 years ago by mhansen

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 aly.deines

comment:5 Changed 9 years ago by aly.deines

I found and fixed error in the doctest.

comment:6 Changed 9 years ago by rkirov

  • Cc John Cremona rkirov added; John Cremona removed

comment:7 Changed 9 years ago by rkirov

  • Cc cremona added; John Cremona removed

comment:8 Changed 9 years ago by annahaensch

  • 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 mpatel

  • Merged in set to sage-4.5.2.alpha0
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:10 Changed 8 years ago by aly.deines

  • Authors changed from Alyson Deines, Radoslav Kirov to Aly Deines, Radoslav Kirov
Note: See TracTickets for help on using tickets.