Opened 14 years ago

Closed 13 years ago

#590 closed enhancement (fixed)

[with patch, positive review] document extended_rational_field.py

Reported by: was Owned by: mhansen
Priority: major Milestone: sage-2.10.3
Component: basic arithmetic Keywords:
Cc: Merged in:
Authors: Reviewers:
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

The rings/extended_rational_field.py file is terribly documented. There are no doctests, no copyright notice, no author, etc. I think David Roe wrote this:

was@ubuntu:~/d/sage/sage/rings$ sage -coverage extended_rational_field.py


extended_rational_field.py ERROR: Please define a s == loads(dumps(s)) doctest. OVERALL SCORE: 0% (2 good, 71 bad)

Missing documentation:

  • init(self)
  • _repr_(self)
  • _latex_(self)
  • call(self, x, base = 0)
  • _coerce_impl(self, x)
  • _is_valid_homomorphism(self, codomain, im_gens)
  • iter(self)
  • complex_embedding(self, prec=53)
  • gens(self)
  • gen(self, n=0)
  • is_prime_field(self)
  • ngens(self)
  • numberfield(self, poly_var, nf_var)
  • init(self, x = None, base = 0)
  • cmp(self, other)
  • copy(self)
  • lcm(self, other)
  • square_root(self)
  • nth_root(self)
  • _add_(self, right)
  • _sub_(self, right)
  • _neg_(self)
  • _mul_(self, right)
  • _div_(self, right)
  • invert(self)
  • pow(self, n)
  • abs(self)
  • floor(self)
  • ceil(self)
  • lshift(self, n)
  • rshift(self, n)
  • init(self)
  • cmp(self, other)
  • repr(self)
  • _latex_(self)
  • _add_(self, other)
  • _mul_(self, other)
  • _sub_(self, other)
  • _div_(self, other)
  • _neg_(self)
  • invert(self)
  • abs(self)
  • pow(self, right)
  • sqrt(self)
  • square_root(self)
  • nth_root(self, n)
  • floor(self)
  • ceil(self)
  • numerator(self)
  • denominator(self)
  • init(self)
  • cmp(self, other)
  • _repr_(self)
  • _latex_(self)
  • _add_(self, other)
  • _mul_(self, other)
  • _sub_(self, other)
  • _div_(self, other)
  • _neg_(self)
  • invert(self)
  • abs(self)
  • pow(self, right)
  • sqrt(self)
  • square_root(self)
  • nth_root(self, n)
  • floor(self)
  • ceil(self)
  • numerator(self)
  • denominator(self)

Missing doctests:

  • numerator(self)
  • denominator(self)

Attachments (2)

590.patch (39.5 KB) - added by mhansen 13 years ago.
590-referee.patch (3.3 KB) - added by mhansen 13 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 13 years ago by mhansen

  • Owner changed from roed to mhansen
  • Status changed from new to assigned

comment:2 Changed 13 years ago by mhansen

  • Summary changed from document extended_rational_field.py to [with patch, needs review] document extended_rational_field.py

Changed 13 years ago by mhansen

comment:3 Changed 13 years ago by dmharvey

  • Summary changed from [with patch, needs review] document extended_rational_field.py to [with patch, mostly positive review, some questions to answer] document extended_rational_field.py

I'm mostly happy with this patch; I have a few questions.

  • What is IntegerWrapper? That's been added with no explanation at all, and I don't understand its purpose. Is it really necessary? If so, there needs to be some documentation.
  • regarding coerce_map_from_impl and Q_to_ExtendedQ: I don't understand the coercion framework any more, so I can't vouch for correctness of the implementations. I'd like someone who understands coercion to take a quick look. Mike, if you find someone on IRC who can sign off on these, that's fine. One thing that bothers me slightly is:
sage: ExtendedRationalField.coerce_map_from_impl(ExtendedIntegerRing)
[boom]
  • docstring for ExtendedRational.__init__ is a little confusing; "The class of extended rational numbers" is a little confusing, sounds like "The set of extended rational numbers". Perhaps better something like "Constructor for elements of the extended rational field".
  • _mul_: I'd like to see examples of multiplying infinity by infinity and minus infinity

I have various other complaints about this module, but it's not in the new code you've written and I don't want to hold up this patch, so I won't go into them now.

comment:4 Changed 13 years ago by gfurnish

I'm happy with coerce_map_from_impl and Q_to_ExtendedQ, but I think _coerce_impl needs to by default check if its in ExtendedZZ then see if it can be coerced into QQ, then error out. Current code may not work with things that can be in ExtendedQQ but are not typed as integers (3 in RR).

comment:5 Changed 13 years ago by gfurnish

  • Summary changed from [with patch, mostly positive review, some questions to answer] document extended_rational_field.py to [with patch, mostly positive review, needs patch] document extended_rational_field.py

comment:6 Changed 13 years ago by mhansen

I attached a patch addressing the referee's concerns.

comment:7 Changed 13 years ago by dmharvey

Ummm, the doctests for sage/rings/extended_integer_ring.py do not pass for me with this patch.

comment:8 Changed 13 years ago by mhansen

Which ones fail? What version are you applying against? If it's the cmp ones, it may just be something random due to architecture differences.

comment:9 Changed 13 years ago by dmharvey

It's mac os 10.4.11 intel. Here's the failure:

sage -t  devel/sage-590/sage/rings/extended_integer_ring.py **********************************************************************
File "extended_integer_ring.py", line 58:
    sage: cmp(ExtendedIntegerRing, ExtendedRationalField)
Expected:
    1
Got:
    -1
**********************************************************************

Why would cmp be architecture-dependent? Is it comparing pointers somewhere or something stupid like that?

comment:10 Changed 13 years ago by mhansen

Yep. Python like to be able to compare everything. With the new coercion stuff coming in, things will have more meaningful _cmp_ functions.

Changed 13 years ago by mhansen

comment:11 Changed 13 years ago by dmharvey

  • Summary changed from [with patch, mostly positive review, needs patch] document extended_rational_field.py to [with patch, accepted] document extended_rational_field.py

okay, I'm happy now.

comment:12 Changed 13 years ago by mhansen

  • Summary changed from [with patch, accepted] document extended_rational_field.py to [with patch, positive review] document extended_rational_field.py

comment:13 Changed 13 years ago by mabshoff

  • Resolution set to fixed
  • Status changed from assigned to closed

Merged both patches in Sage 2.10.3.rc1

Note: See TracTickets for help on using tickets.