Opened 7 months ago
Closed 7 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, GitHub, GitLab) | Commit: | 339781ab5b478be8ccf4115e085ab09707f9a8e8 |
Dependencies: | Stopgaps: |
Description (last modified by )
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 7 months ago by
- 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 7 months ago by
- Component changed from PLEASE CHANGE to manifolds
comment:3 Changed 7 months ago by
- Description modified (diff)
comment:4 Changed 7 months ago by
- Description modified (diff)
- Status changed from new to needs_review
comment:5 Changed 7 months ago by
- Branch set to u/gh-mjungmath/scalar_field_arithmetics__trivial_cases
comment:6 Changed 7 months ago by
- Commit set to 2446f7ecf68fe4034c8f92f6248cd877b2efe6f3
- Description modified (diff)
comment:7 Changed 7 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 7 months ago by
comment:9 Changed 7 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 7 months ago by
- Cc mkoeppe added
I am also curious about your opinion on my very last comment, Matthias.
comment:11 Changed 7 months ago by
Wrong ticket, sorry Matthias...I was referring to #30239. But you're already in the cc there.
comment:12 Changed 7 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 7 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 7 months ago by
You mean something like this?
comment:15 Changed 7 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 7 months ago by
I will do a short performance check tomorrow and post it here.
comment:17 Changed 7 months ago by
- Status changed from needs_review to positive_review
Thanks Travis.
Performance check follows...
comment:18 Changed 7 months ago by
comment:19 Changed 7 months ago by
comment:20 Changed 7 months ago by
- Description modified (diff)
comment:21 Changed 7 months ago by
- Branch changed from u/gh-mjungmath/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_