Opened 6 months ago

Closed 6 months ago

#22859 closed enhancement (fixed)

Do not check for a zero result in arithmetics of coordinate functions and scalar fields

Reported by: egourgoulhon Owned by:
Priority: major Milestone: sage-8.0
Component: geometry Keywords: coordinate function, scalar field, manifold
Cc: tscrim Merged in:
Authors: Eric Gourgoulhon Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 94e8c68 (Commits) Commit: 94e8c68536ffc0027c6f82177b10140dc43e58ef
Dependencies: Stopgaps:

Description (last modified by egourgoulhon)

In the arithmetics of symbolic coordinate functions (methods sage.manifolds.coord_func_symb.CoordFunctionSymb._add_, _sub_, _mul_ and _div_), the final result (res below) is checked against zero as follows:

if res == 0:
    return self.parent().zero()

Now, for complicated expressions, such a check involves Maxima and is very slow. Moreover, there is no real benefit in representing all the zero functions by a unique object (the zero of the function ring). This ticket proposes therefore to remove the check. It also removes a similar check in the arithmetics of scalar fields (class sage.manifolds.scalarfield.ScalarField).

The gain in performance is quite significant: sage -tp --long src/sage/manifolds/ (with 8 cores) yields

Total time for all tests: 245.9 seconds
    cpu time: 1570.5 seconds
    cumulative wall time: 1689.6 seconds

instead of

Total time for all tests: 300.5 seconds
    cpu time: 1757.9 seconds
    cumulative wall time: 1876.0 seconds

On computationally demanding worksheets, like the Kerr spacetime one, the gain is even greater: the total time for running all cells is 386 s, instead of 610 s.

Besides, the method is_zero of class CoordFunctionSymb is replaced by a method __bool__ (with an alias to __nonzero__ for Python 2 compatibility), leaving the method is_zero only in the base class sage.structure.element.Element.

Change History (9)

comment:1 Changed 6 months ago by egourgoulhon

  • Branch set to public/manifolds/coord_function_arithm-22859
  • Commit set to 0960e64cbaf86ba418716fed4e41384c948c4eed
  • Status changed from new to needs_review

New commits:

0960e64Do not check for a zero result in arithmetics of coordinate functions and scalar fields

comment:2 Changed 6 months ago by egourgoulhon

  • Description modified (diff)

comment:3 Changed 6 months ago by egourgoulhon

  • Cc tscrim added
  • Description modified (diff)

comment:4 follow-up: Changed 6 months ago by tscrim

The question comes down to checking whether the element represents zero or if constructing a new 0 element is faster. There is also the x.is_trivial_zero(), which might catch some of the cases. Additionally, am I correct is that is_zero returns not self? Otherwise it LGTM.

comment:5 Changed 6 months ago by git

  • Commit changed from 0960e64cbaf86ba418716fed4e41384c948c4eed to 94e8c68536ffc0027c6f82177b10140dc43e58ef

Branch pushed to git repo; I updated commit sha1. New commits:

94e8c68Use of is_trivial_zero in arithmetics of coordinate functions

comment:6 in reply to: ↑ 4 Changed 6 months ago by egourgoulhon

Replying to tscrim:

The question comes down to checking whether the element represents zero or if constructing a new 0 element is faster.

Indeed.

There is also the x.is_trivial_zero(), which might catch some of the cases.

Thanks for pointing out is_trivial_zero(); I've restored checks by means of this function in the above commit. For sage -tp --long src/sage/manifolds/, there is no noticeable difference and for the Kerr metric worksheet, the total time becomes 378 s, instead of 386 s. So I think we can buy it!

Additionally, am I correct is that is_zero returns not self?

Yes, it is defined like this in src/sage/structure/element.pyx and it is that method which is inheritated by coordinate functions.

comment:7 Changed 6 months ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

Ah, right, it is a subclass of Element. Thanks.

comment:8 Changed 6 months ago by egourgoulhon

Many thanks for the review!

comment:9 Changed 6 months ago by vbraun

  • Branch changed from public/manifolds/coord_function_arithm-22859 to 94e8c68536ffc0027c6f82177b10140dc43e58ef
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.