Opened 22 months ago
Closed 21 months ago
#28554 closed enhancement (fixed)
Scalar Field Restrictions
Reported by:  ghDeRhamSource  Owned by:  

Priority:  major  Milestone:  sage9.0 
Component:  geometry  Keywords:  manifolds, scalar fields 
Cc:  tscrim, egourgoulhon  Merged in:  
Authors:  Michael Jung  Reviewers:  Eric Gourgoulhon 
Report Upstream:  N/A  Work issues:  
Branch:  a126d91 (Commits, GitHub, GitLab)  Commit:  a126d9187fed8552fd14351248f54704dac26737 
Dependencies:  Stopgaps: 
Description (last modified by )
How is a scalar field implemented which is piecewisely defined with different expressions in one particular chart?
Take for instance a scalar field f
on the real line with standard "top" chart x
, defined via f(x)=0 for x<1
, f(x)=x+1 for 1<=x<0
, f(x)=1x for 0<=x<1
and f(x)=0 for x>=1
. Currently, this is solved by using
f = M.scalar_field( unit_step(x + 1)*unit_step(1  x)*(1  abs(x)) )
(see https://trac.sagemath.org/ticket/28519#comment:46).
This solution is quite unhandy and becomes even more so for more complicated scalar fields.
This ticket is part of the metaticket #28519.
This ticket includes:
display
method modified in such a way that all distinct expressions are shown (a small slowdown in computation time)set_restriction
method added smilar to tensor fields
Change History (22)
comment:1 Changed 22 months ago by
comment:2 Changed 22 months ago by
 Branch set to u/ghDeRhamSource/scalar_field_restrictions
comment:3 Changed 22 months ago by
 Commit set to 0c645ca987eb13c63cfc7841aac1c4d320eec253
 Description modified (diff)
New commits:
0c645ca  'set_restriction' added + 'display' modified + naming modified

comment:4 Changed 22 months ago by
 Description modified (diff)
comment:5 Changed 22 months ago by
I compared computation times.
Before:
...$ ./sage t src/sage/manifolds/scalarfield.py too few successful tests, not using stored timings Running doctests with ID 20191004190338e810720b. Git branch: scalar_field_restrictions Using optional=build,dochtml,memlimit,python2,sage Doctesting 1 file. sage t src/sage/manifolds/scalarfield.py [715 tests, 31.77 s]  All tests passed!  Total time for all tests: 32.0 seconds cpu time: 33.2 seconds cumulative wall time: 31.8 seconds
With this ticket:
...$ ./sage t src/sage/manifolds/scalarfield.py too few successful tests, not using stored timings Running doctests with ID 20191004204512df0ec794. Git branch: scalar_field_restrictions Using optional=build,dochtml,memlimit,python2,sage Doctesting 1 file. sage t src/sage/manifolds/scalarfield.py [735 tests, 34.59 s]  All tests passed!  Total time for all tests: 34.8 seconds cpu time: 35.9 seconds cumulative wall time: 34.6 seconds
What do you say?
comment:6 Changed 22 months ago by
 Commit changed from 0c645ca987eb13c63cfc7841aac1c4d320eec253 to b85f6ec46f9878c48ed4d8843123b46b844cb252
Branch pushed to git repo; I updated commit sha1. New commits:
b85f6ec  Typos fixed

comment:7 Changed 22 months ago by
 Commit changed from b85f6ec46f9878c48ed4d8843123b46b844cb252 to 1d68a1f83e9fbb0135077e5e0ec60be0f5beb141
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
1d68a1f  'set_restriction' added + some restriction modifications

comment:8 Changed 22 months ago by
 Description modified (diff)
comment:9 Changed 22 months ago by
 Description modified (diff)
comment:10 Changed 22 months ago by
Please give me a short feedback so that I can work further on this.
This ticket is essential for my future implementation of characteristic classes (due to the set_restriction
method).
comment:11 Changed 22 months ago by
 Status changed from new to needs_info
comment:12 Changed 21 months ago by
Sorry for the delay. The ticket looks good; it clearly improves the treatment of restrictions of scalar fields. I have a few remarks:
 Regarding the change in
display()
, I agree that your solution is a good compromise  I guess the method
_new_instance()
has been introduced in scalar fields by analogy of what is done for tensor fields. However in the present case, this method is quite trivial, since it returns a meretype(self)(self.parent())
. IMHO this adds some code complexity (a new method) for not a big gain; for someone reading the code,res = type(self)(self.parent())
is at least as clear asres = self._new_instance()
Moreover the latter involves one extra function call. Therefore I would suggest to revert this change.
 When you perform significant changes/additions to some file, I would suggest that you add your name to the
AUTHORS
field and copyright statement. For instance insrc/sage/mnifolds/scalarfield.py
, you could add to theAUTHORS
field something like Michael Jung (2019) : improve restrictions; make ``display()`` show all distinct expressions
The same holds for #28628 and other tickets of #28519.
comment:13 Changed 21 months ago by
 Commit changed from 1d68a1f83e9fbb0135077e5e0ec60be0f5beb141 to 512d2b1a31d7491e5ecce1bb167b74830929b06d
comment:14 Changed 21 months ago by
 Status changed from needs_info to needs_review
Thanks! There we go. :)
comment:15 Changed 21 months ago by
 Status changed from needs_review to needs_work
With Sage 9.0.beta3, there are two failed doctests:
********************************************************************** File "src/sage/manifolds/section.py", line 971, in sage.manifolds.section.Section.add_expr_from_subdomain Failed example: sorted(s._components.values())[0]._comp[(1,)].display() Expected: S^2 > R on U: (x, y) > x Got: S^2 > R on U: (x, y) > x on W: (u, v) > u/(u^2 + v^2) ********************************************************************** ********************************************************************** File "src/sage/manifolds/differentiable/pseudo_riemannian_submanifold.py", line 1517, in sage.manifolds.differentiable.pseudo_riemannian_submanifold.?.principal_curvatures Failed example: N.principal_curvatures(stereoN)[0].display() # long time Expected: k_0: N > R on U: x > 1 Got: k_0: N > R on U: x > 1 on W: y > 1 **********************************************************************
comment:16 Changed 21 months ago by
Btw, thanks for reverting _new_instance()
.
comment:17 Changed 21 months ago by
 Commit changed from 512d2b1a31d7491e5ecce1bb167b74830929b06d to a126d9187fed8552fd14351248f54704dac26737
comment:18 Changed 21 months ago by
I totally missed that we are in beta3 already!
Please check once again.
Thanks for your effort so far! :)
comment:19 Changed 21 months ago by
 Status changed from needs_work to needs_review
comment:20 Changed 21 months ago by
 Status changed from needs_review to positive_review
Everything is fine now. Thanks.
comment:21 Changed 21 months ago by
 Reviewers set to Eric Gourgoulhon
comment:22 Changed 21 months ago by
 Branch changed from u/ghDeRhamSource/scalar_field_restrictions to a126d9187fed8552fd14351248f54704dac26737
 Resolution set to fixed
 Status changed from positive_review to closed
Eric, in the ticket mentioned above, I suggested to display all known expressions (on the greatest domain) and compute new expressions only if a particular chart is stated. It seems, there is no big difference to the previous code then, and the additional computation time might be negligible. What do you say? Is this a good compromise for you? Moreover, this approach seems to be the easiest one.
Or one implements an additional flag
display_all
for which the user is aware of additional computation time when using it?However, do you have an example for me with much computation time to test my current algorithm how long it takes? Then, I can compare computation times. I have a feeling that the current algorithm is not that much slower than before because it starts from the top charts  what happens anyway and, I guess, it only makes a significant difference if the scalar field is picewisely defined.