Opened 4 months ago

Closed 4 months ago

#30291 closed enhancement (fixed)

Scalar Field Arithmetics: Trivial Cases

Reported by: gh-mjungmath Owned by:
Priority: major Milestone: sage-9.2
Component: manifolds Keywords:
Cc: egourgoulhon, tscrim, mkoeppe Merged in:
Authors: Michael Jung Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 339781a (Commits) Commit: 339781ab5b478be8ccf4115e085ab09707f9a8e8
Dependencies: Stopgaps:

Description (last modified by gh-mjungmath)

Setup:

sage: M = Manifold(2, 'M', structure='topological') # the 2-dimensional sphere S^2
sage: U = M.open_subset('U') # complement of the North pole
sage: c_xy.<x,y> = U.chart() # stereographic coordinates from the North pole
sage: V = M.open_subset('V') # complement of the South pole
sage: c_uv.<u,v> = V.chart() # stereographic coordinates from the South pole
sage: M.declare_union(U,V)   # S^2 is the union of U and V
sage: xy_to_uv = c_xy.transition_map(c_uv, (x/(x^2+y^2), y/(x^2+y^2)),
....:                                intersection_name='W',
....:                                restrictions1= x^2+y^2!=0,
....:                                restrictions2= u^2+v^2!=0)
sage: uv_to_xy = xy_to_uv.inverse()
sage: f = M.scalar_field({c_xy: 1/(1+x^2+y^2), c_uv: (u^2+v^2)/(1+u^2+v^2)},
....:                    name='f')

Current state:

sage: %timeit M.one_scalar_field()*f
10 loops, best of 5: 47.5 ms per loop
sage: %timeit f*M.one_scalar_field()
10 loops, best of 5: 48 ms per loop

This ticket:

sage: %timeit M.one_scalar_field()*f
The slowest run took 4.05 times longer than the fastest. This could mean that an intermediate result is being cached.
10000 loops, best of 5: 21.1 µs per loop
sage: %timeit f*M.one_scalar_field()
10000 loops, best of 5: 26.5 µs per loop

But the generic multiplication seems not affected:

Current state:

sage: %timeit f*f
10 loops, best of 5: 77.8 ms per loop

This ticket:

sage: %timeit f*f
10 loops, best of 5: 77.4 ms per loop

The output is improved, too:

Current state:

sage: M.one_scalar_field()*M.one_scalar_field()
Scalar field 1*1 on the 2-dimensional topological manifold M

This ticket:

sage: M.one_scalar_field()*M.one_scalar_field()
Scalar field 1 on the 2-dimensional topological manifold M

Change History (21)

comment:1 Changed 4 months ago by gh-mjungmath

  • Authors set to Michael Jung
  • Cc egourgoulhon tscrim added
  • Description modified (diff)
  • Summary changed from try-except block in display method to Scalar Field Arithmetics: Trivial Cases
  • Type changed from PLEASE CHANGE to enhancement

comment:2 Changed 4 months ago by gh-mjungmath

  • Component changed from PLEASE CHANGE to manifolds

comment:3 Changed 4 months ago by gh-mjungmath

  • Description modified (diff)

comment:4 Changed 4 months ago by gh-mjungmath

  • Description modified (diff)
  • Status changed from new to needs_review

comment:5 Changed 4 months ago by gh-mjungmath

  • Branch set to u/gh-mjungmath/scalar_field_arithmetics__trivial_cases

comment:6 Changed 4 months ago by gh-mjungmath

  • Commit set to 2446f7ecf68fe4034c8f92f6248cd877b2efe6f3
  • Description modified (diff)

New commits:

2446f7eTrac 30291: simple checks for trivial cases in _mul_, _add_ and _div_

comment:7 Changed 4 months ago by git

  • Commit changed from 2446f7ecf68fe4034c8f92f6248cd877b2efe6f3 to 4bd82e47382226353261431b505dc3c557e13c47

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

4bd82e4Trac #30291: patchbot doctest fixed

comment:8 Changed 4 months ago by gh-mjungmath

Apparently, I mixed some things up with #30239. I'll rebase; same for #30239.

Last edited 4 months ago by gh-mjungmath (previous) (diff)

comment:9 Changed 4 months ago by git

  • Commit changed from 4bd82e47382226353261431b505dc3c557e13c47 to 4f3b69f6159dbdc127070fce043a3428e09326c0

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

4f3b69fTrac 30291: simple checks for trivial cases in _mul_, _add_ and _div_

comment:10 Changed 4 months ago by gh-mjungmath

  • Cc mkoeppe added

I am also curious about your opinion on my very last comment, Matthias.

comment:11 Changed 4 months ago by gh-mjungmath

Wrong ticket, sorry Matthias...I was referring to #30239.

Version 1, edited 4 months ago by gh-mjungmath (previous) (next) (diff)

comment:12 Changed 4 months ago by tscrim

Note that (self - 1) will involve a coercion call. How difficult would it be to instead add a method is_trivial_one that just pushes the x - self.base_ring().one() (or maybe x - SR.one()) computation to the essential code to avoid creating fewer temporary (heavy) objects?

comment:13 Changed 4 months ago by git

  • Commit changed from 4f3b69f6159dbdc127070fce043a3428e09326c0 to 339781ab5b478be8ccf4115e085ab09707f9a8e8

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

339781aTrac #30291: add is_trivial_one() method

comment:14 Changed 4 months ago by gh-mjungmath

You mean something like this?

comment:15 Changed 4 months ago by tscrim

  • Reviewers set to Travis Scrimshaw

Yep, exactly. This seems like it should be faster than using (self - 1).is_trivial_zero(). If the the patchbot comes back green, then you can set a positive review.

comment:16 Changed 4 months ago by gh-mjungmath

I will do a short performance check tomorrow and post it here.

Last edited 4 months ago by gh-mjungmath (previous) (diff)

comment:17 Changed 4 months ago by gh-mjungmath

  • Status changed from needs_review to positive_review

Thanks Travis.

Performance check follows...

comment:18 Changed 4 months ago by gh-mjungmath

Last edited 4 months ago by gh-mjungmath (previous) (diff)

comment:19 Changed 4 months ago by gh-mjungmath

Last edited 4 months ago by gh-mjungmath (previous) (diff)

comment:20 Changed 4 months ago by gh-mjungmath

  • Description modified (diff)

comment:21 Changed 4 months ago by vbraun

  • Branch changed from u/gh-mjungmath/scalar_field_arithmetics__trivial_cases to 339781ab5b478be8ccf4115e085ab09707f9a8e8
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.