Opened 16 months ago
Closed 16 months ago
#30291 closed enhancement (fixed)
Scalar Field Arithmetics: Trivial Cases
Reported by:  ghmjungmath  Owned by:  

Priority:  major  Milestone:  sage9.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, GitHub, GitLab)  Commit:  339781ab5b478be8ccf4115e085ab09707f9a8e8 
Dependencies:  Stopgaps: 
Description (last modified by )
Setup:
sage: M = Manifold(2, 'M', structure='topological') # the 2dimensional 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 2dimensional topological manifold M
This ticket:
sage: M.one_scalar_field()*M.one_scalar_field() Scalar field 1 on the 2dimensional topological manifold M
Change History (21)
comment:1 Changed 16 months ago by
 Cc egourgoulhon tscrim added
 Description modified (diff)
 Summary changed from tryexcept block in display method to Scalar Field Arithmetics: Trivial Cases
 Type changed from PLEASE CHANGE to enhancement
comment:2 Changed 16 months ago by
 Component changed from PLEASE CHANGE to manifolds
comment:3 Changed 16 months ago by
 Description modified (diff)
comment:4 Changed 16 months ago by
 Description modified (diff)
 Status changed from new to needs_review
comment:5 Changed 16 months ago by
 Branch set to u/ghmjungmath/scalar_field_arithmetics__trivial_cases
comment:6 Changed 16 months ago by
 Commit set to 2446f7ecf68fe4034c8f92f6248cd877b2efe6f3
 Description modified (diff)
comment:7 Changed 16 months ago by
 Commit changed from 2446f7ecf68fe4034c8f92f6248cd877b2efe6f3 to 4bd82e47382226353261431b505dc3c557e13c47
Branch pushed to git repo; I updated commit sha1. New commits:
4bd82e4  Trac #30291: patchbot doctest fixed

comment:8 Changed 16 months ago by
comment:9 Changed 16 months ago by
 Commit changed from 4bd82e47382226353261431b505dc3c557e13c47 to 4f3b69f6159dbdc127070fce043a3428e09326c0
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
4f3b69f  Trac 30291: simple checks for trivial cases in _mul_, _add_ and _div_

comment:10 Changed 16 months ago by
 Cc mkoeppe added
I am also curious about your opinion on my very last comment, Matthias.
comment:11 Changed 16 months ago by
Wrong ticket, sorry Matthias...I was referring to #30239. But you're already in the cc there.
comment:12 Changed 16 months ago by
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 16 months ago by
 Commit changed from 4f3b69f6159dbdc127070fce043a3428e09326c0 to 339781ab5b478be8ccf4115e085ab09707f9a8e8
Branch pushed to git repo; I updated commit sha1. New commits:
339781a  Trac #30291: add is_trivial_one() method

comment:14 Changed 16 months ago by
You mean something like this?
comment:15 Changed 16 months ago by
 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 16 months ago by
I will do a short performance check tomorrow and post it here.
comment:17 Changed 16 months ago by
 Status changed from needs_review to positive_review
Thanks Travis.
Performance check follows...
comment:19 Changed 16 months ago by
comment:20 Changed 16 months ago by
 Description modified (diff)
comment:21 Changed 16 months ago by
 Branch changed from u/ghmjungmath/scalar_field_arithmetics__trivial_cases to 339781ab5b478be8ccf4115e085ab09707f9a8e8
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Trac 30291: simple checks for trivial cases in _mul_, _add_ and _div_