#19152: {Real,Complex}Ball: Miscellaneous fixes and improvements
-------------------------------------+-------------------------------------
Reporter: mmezzarobba | Owner:
Type: enhancement | Status: needs_work
Priority: major | Milestone: sage-6.10
Component: numerical | Resolution:
Keywords: | Merged in:
Authors: Marc Mezzarobba | Reviewers: Clemens Heuberger
Report Upstream: N/A | Work issues:
Branch: u/cheuberg/19152 | Commit:
-arb-misc | 16a04a07c63349d3792ed8c538a7d60e5adfde5e
Dependencies: #19063 | Stopgaps:
-------------------------------------+-------------------------------------
Comment (by mmezzarobba):
Hello Clemens,
Thanks again for your comments and the changes you made.
Replying to [comment:12 cheuberg]:
> I read the modifications and have the following comments.
> 1. `arf.pxd`: You define `arf_rnd_t` as an enum and rely on the fact
that the numeric values of `ARF_RND_DOWN` etc. will never change in the C
header files. However, there, the values are macros, so I do not know
whether we can safely assume the values never to change.
I fear I don't understand what you mean here, sorry. I thought that cython
would insert textual references to `ARF_RND_*` in the generated C code.
Isn't that the case?
> 2. `polynomial_element.pyx`:
> - I am very surprised to see this change here in an arb ticket.
I tend to consider that what matters is commits, not tickets. So I don't
have any problem with that as long as it doesn't make the review harder,
and I actually feel if it makes reviews easier... But please tell me if
you don't like that and I will try to create separate tickets in the
future.
> - The documentation of `is_gen` states: "Important - this function
doesn’t return True if self equals the generator; it returns True if self
is the generator." I.e., the outcome differs from the old behaviour.
Good catch! I'd say it is okay, because the operation that really should
be fast IMO is `x^n` where `x = Pol.gen()`, not `(2*x - x)^n`, and the new
version seems to be about 10% faster when `n == 2`. But I'm open to
suggestions.
> 4. `RealBall.__hash__`: I do not like the fact that the fractional part
of the midpoint is ignored completely:
> {{{
> sage: hash(RBF(3/4))
> 1048577
> sage: hash(RBF(5/8))
> 1048577
> sage: hash(RBF(7/8))
> 1048577
> }}}
Yes, I don't know what I was doing with that hash!
> 5. `RealBall.upper`, `RealBall.lower`, `RealBall.endpoints`: missing
INPUT: and OUTPUT: sections, document and test parameter `rnd`
The problem here is that, unlike elements of `RealIntervalField(p)`,
elements of `RealBallField(p)` may have endpoints that are not be
representable on `p` bits. I wasn't sure what to do in this case, so I
implemented a quick wrapper for the interval version and then forgot to
come back to it. (This is also the reason for the clumsy formulation
“viewed as an interval” Jeroen asks about.)
The main options I see are:
- either to return the endpoints rounded (in which direction? we can't be
100% compatible with interval fields!) to the parent's precision,
- or return the exact endpoints, but then, the output parent will be hard
to predict.
Which do you think is best?
> 6. `RealBall.add_error`: missing INPUT: and OUTPUT: sections
What would you suggest to say there?
> 7. `RealBall.is_nonzero`: The note section is hard to understand because
`__nonzero__` and `is_zero` should do very different things.
I'm not sure I understand your comment. As far as I understand (but all
this stuff is very badly documented!), in the rest of Sage,
`__nonzero__()` means “syntactically nonzero” (for example, the `degree()`
methods of polynomials look for the leading `__nonzero__` term), which for
balls is exactly the negation of `is_zero()`.
> 8. `RealBall.__lshift__`, `RealBall.__rshift__`: why is the first
parameter called `val` instead of `self` and how can it happen that it is
not a `RealBall` if this is a method of `RealBall`?
That's how special methods work in Cython... See
http://docs.cython.org/src/userguide/special_methods.html#arithmetic-
methods
> 9. `ComplexBall._is_atomic`: `self.is_real() or self.imag().is_zero()`
seems to be redundant. Do you want to say `self.real().is_zero()` instead?
I think so, yes, thanks! But actually I don't understand what
`_is_atomic()` ''really'' should do: for example, the documentation seems
to suggest that `-2` should not be atomic (since `x + -2` is not correct),
while it is atomic in various standard rings, and I think it has to in
order to avoid redundant parentheses...
--
Ticket URL: <http://trac.sagemath.org/ticket/19152#comment:17>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at http://groups.google.com/group/sage-trac.
For more options, visit https://groups.google.com/d/optout.
