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: |
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)
Change History (15)
comment:1 Changed 13 years ago by
- Owner changed from roed to mhansen
- Status changed from new to assigned
comment:2 Changed 13 years ago by
- Summary changed from document extended_rational_field.py to [with patch, needs review] document extended_rational_field.py
Changed 13 years ago by
comment:3 Changed 13 years ago by
- 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
comment:4 Changed 13 years ago by
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
- 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
I attached a patch addressing the referee's concerns.
comment:7 Changed 13 years ago by
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
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
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
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
comment:11 Changed 13 years ago by
- 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
- 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
- Resolution set to fixed
- Status changed from assigned to closed
Merged both patches in Sage 2.10.3.rc1
I'm mostly happy with this patch; I have a few questions.
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.coerce_map_from_impl
andQ_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: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 infinityI 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.