#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 jdemeyer):
I am having a quick look though this branch, mostly to check for
technicalities. It's a lot, so I'm not saying that you really must fix all
these for positive_review.
1. The naming of the files is inconsistent: why `real_arb` and
`complex_ball_acb`?
2. You can remove the `include_dirs=[SAGE_INC + '/flint'])` from
`src/module_list.py`.
3. Ideally, the type declarations in the arb `.pxd` files should be in a
different file: try to follow the model of `src/sage/libs/gmp` for
example. Then you should add `# distutils: libraries = arb` to the files
with library functions (i.e. not types).
4. The `__hash__` for `RBF` is bad:
{{{
sage: from sage.rings.real_arb import RBF
sage: hash(RBF(0.6))
1048577
sage: hash(RBF(0.7))
1048577
sage: hash(RBF(0.8))
1048577
sage: hash(RBF(0.9))
1048577
}}}
5. For `CBF.__hash__`, what's the point of `>> 7`?
6. This is a very strange way to handle exceptions, what's wrong with
`raise`?
{{{
if arf_get_mpfr(rad.value, tmp, GMP_RNDN):
abort()
}}}
7. Replace the `GMP_RND` macros by `MPFR_RND`.
8. You can replace
{{{
cdef extern from "math.h":
long lrint(double)
}}}
by `from libc.math cimport lrint` (although I would prefer to not use it
in the hash).
9. What's with this comment?
{{{
# FIXME: RBF is not even associative, but
CompletionFunctor
# only works with rings, and other coercion features
require
# methods like is_integral_domain() to be defined
category=category or
sage.categories.fields.Fields().Infinite())
}}}
Your class models a mathematical field (real or complex numbers), so I
don't see the problem. How would you want to fix this?
10. Now that all this is standard, can you add `RBF`, `RealBallField`,
`CBF` and `ComplexBallField` to the global namespace and then remove all
doctest lines of the form `from sage.rings.real_arb import RBF`.
11. You should really avoid this: `include "sage/ext/python.pxi"`, just
`cimport` what you need.
12. What's the difference between `contains_exact` and `__contains__`? I
don't understand why you need both these methods.
13. When raising exceptions, add a string: don't just `raise TypeError`,
but `raise TypeError("useful message here")`
14. You write `.. SEEALSO:: :meth:`abs` but there is no such method.
15. I still don't understand why `below_abs` and `above_abs` return a ball
instead of a real number. At the very least, this should be documentated
in an `OUTPUT:` block in those methods.
16. This is confusing documentation: `Return the right endpoint of this
ball, viewed as an interval.` I would read it like this method returns an
interval. Why not `Return the right endpoint of this ball.`?
17. I don't understand the purpose of this comment: `# TODO: once CBF is
there, also wrap functions that only exist in acb*`
18. I don't understand what this means: `Return a copy of this ball
rounded to the precision of the parent.`
--
Ticket URL: <http://trac.sagemath.org/ticket/19152#comment:15>
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.
