Opened 11 months ago
Closed 11 months ago
#28628 closed defect (fixed)
Tensor Fields: set_restriction Behaviour
Reported by:  ghDeRhamSource  Owned by:  

Priority:  major  Milestone:  sage9.0 
Component:  geometry  Keywords:  manifolds, tensor 
Cc:  egourgoulhon  Merged in:  
Authors:  Michael Jung  Reviewers:  Eric Gourgoulhon 
Report Upstream:  N/A  Work issues:  
Branch:  9299f89 (Commits)  Commit:  9299f894a11b02671472d356d51605d1e5277cf3 
Dependencies:  Stopgaps: 
Description (last modified by )
The following code leads to an error:
sage: M = Manifold(2, 'M') sage: X.<x,y> = M.chart() sage: eX = X.frame() sage: a = M.one_form() sage: b = M.one_form() sage: a[eX,:] = x*y, x+y sage: b.set_restriction(a) sage: b.display() ValueError Traceback (most recent call last) ... ValueError: no basis could be found for computing the components in the Coordinate frame (M, (d/dx,d/dy))
Mathematically, this should not happen.
A method copy_from
is added, and in case of the same domain, the copy_from
method is invoked, when set_restriction
is used.
This ticket is part of the metaticket #28519.
Change History (37)
comment:1 Changed 11 months ago by
 Branch set to u/ghDeRhamSource/tensor_fields__set_restriction_behaviour
comment:2 Changed 11 months ago by
 Cc egourgoulhon added
 Commit set to e30ec5b88f8c3144c08e731d85c4c79e58595cc4
 Description modified (diff)
 Keywords manifolds added
 Status changed from new to needs_review
 Type changed from PLEASE CHANGE to defect
comment:3 Changed 11 months ago by
 Commit changed from e30ec5b88f8c3144c08e731d85c4c79e58595cc4 to 88c31f99d477220b17dea0795deea481b9660f49
Branch pushed to git repo; I updated commit sha1. New commits:
88c31f9  Clear components when 'copy_from' is invoked

comment:4 Changed 11 months ago by
 Component changed from PLEASE CHANGE to geometry
 Status changed from needs_review to needs_work
Thanks for opening the ticket and implementing the fix (the introduction of copy_from
seems a good idea). However I don't think that you should add the optional argument del_restrictions
in TensorField._del_derived
. At first glance, this makes this method on the same footing than TensorFieldParal._del_derived
, which has such an argument. However, TensorField
and TensorFieldParal
are not symmetrical as far as restrictions are concerned (see Sec. 3.5 of these notes):
 for a
TensorFieldParal
, the primary data are the components w.r.t. to vector frames on its domain, stored in the dict_components
; the restrictions are derived quantities, which can be reconstructed at any time from_components
 for a
TensorField
, the primary data are the restrictions to subdomains; they are not derived quantities; deleting them in_del_derived()
would erase essential piece of information on the tensor field
comment:5 Changed 11 months ago by
Actually, I'm aware of that and definitely see your point. Though, as one can see in add_expr_by_continuation
(and now copy_from
), this behaviour is wanted at some point, even for TensorField
objects. For that reason, I put del_restrictions=False
as default, so that the previous code is not affected at all.
If you don't want to have this snippet in _del_derived
, where it would make sense in my opinion, I'd suggest a new method _del_restrictions
for it is used quite often. This simplifies future modifications on this matter (if needed).
What do you say?
comment:6 Changed 11 months ago by
Ah! For add_expr_by_continuation
you want exactly the behaviour you explained, of course. Sorry! However, imho the modifications above make things easier and the code more elegant and consistent.
comment:7 Changed 11 months ago by
 Description modified (diff)
comment:8 Changed 11 months ago by
 Commit changed from 88c31f99d477220b17dea0795deea481b9660f49 to 672d2683ddbde940b528bdeb578d7d3c09892a1f
Branch pushed to git repo; I updated commit sha1. New commits:
672d268  Reorganization of restriction deletion

comment:9 followup: ↓ 10 Changed 11 months ago by
 Status changed from needs_work to needs_review
Is this better?
comment:10 in reply to: ↑ 9 ; followup: ↓ 11 Changed 11 months ago by
Replying to ghDeRhamSource:
Is this better?
Much better! ;)
A question though: what's the point of
TensorField.copy_from(self, other)
in TensorFieldParal.copy_from()
?
Actually, the action of TensorField.copy_from
amounts to copying the restrictions, but the restrictions are deleted by the subsequent call:
FreeModuleTensor.copy_from(self, other)
due to
self._del_derived()
in FreeModuleTensor.copy_from()
.
In view of this, I would simply delete TensorFieldParal.copy_from()
, so that copy_from()
in TensorFieldParal
is inherited from FreeModuleTensor
.
comment:11 in reply to: ↑ 10 ; followup: ↓ 12 Changed 11 months ago by
Replying to egourgoulhon:
Much better! ;)
A question though: what's the point of
TensorField.copy_from(self, other)in
TensorFieldParal.copy_from()
? Actually, the action ofTensorField.copy_from
amounts to copying the restrictions, but the restrictions are deleted by the subsequent call:FreeModuleTensor.copy_from(self, other)due to
self._del_derived()in
FreeModuleTensor.copy_from()
.
Ah! Yes! The order should be switched then, right?
A copy of the restrictions even for TensorFieldParal
would be convenient, wouldn't it?
comment:12 in reply to: ↑ 11 Changed 11 months ago by
Replying to ghDeRhamSource:
Ah! Yes! The order should be switched then, right?
Yes if you would like to maintain TensorFieldParal.copy_from()
with the copies of the restrictions.
A copy of the restrictions even for
TensorFieldParal
would be convenient, wouldn't it?
I am not so sure: for TensorFieldParal
, restrictions are derived quantities, which can be easily reconstructed at any time if necessary. Not copying them makes copy_from()
faster. If we have a.copy_from(b)
with restrictions of b
having been constructed for whatever reasons, it could be that in the future use of a
, no restriction will be invoked. So I would vote for removing copy_from()
from TensorFieldParal
class, keeping it only in FreeModuleTensor
. This makes the code shorter and clearer: copy_from()
does not copy the derived quantities, whatever their nature.
comment:13 Changed 11 months ago by
Okay, that sounds reasonable. I will work on this tomorrow.
By the way, characteristic classes are almost finished. They come with a very rudimentary version of bundle connections. But I have already plans for full bundle connection support. Though, this has to wait since my master thesis deadline runs out soon.
Best wishes, Michael
comment:14 Changed 11 months ago by
 Commit changed from 672d2683ddbde940b528bdeb578d7d3c09892a1f to c790d130e6d99868425ae59198fbe8be1c09769c
Branch pushed to git repo; I updated commit sha1. New commits:
c790d13  'copy_from' removed

comment:15 followup: ↓ 16 Changed 11 months ago by
Done.
comment:16 in reply to: ↑ 15 Changed 11 months ago by
comment:17 Changed 11 months ago by
 Keywords tensor added
 Reviewers set to Eric Gourgoulhon
 Status changed from needs_review to positive_review
comment:18 Changed 11 months ago by
author name missing
comment:19 Changed 11 months ago by
 Status changed from positive_review to needs_work
comment:20 Changed 11 months ago by
 Status changed from needs_work to positive_review
comment:21 followup: ↓ 22 Changed 11 months ago by
 Status changed from positive_review to needs_work
Merge conflict
comment:22 in reply to: ↑ 21 Changed 11 months ago by
comment:23 followup: ↓ 24 Changed 11 months ago by
comment:24 in reply to: ↑ 23 Changed 11 months ago by
Replying to ghDeRhamSource:
No, the affected files are untouched in #28600 and #28159. I rather think it's connected to #28562?
I don't think so, since #28562 is not merged yet. Note that #28159 is touching tensorfield.py
, as the current ticket.
Which code lines cause a merge conflict anyway?
It will be easy to see them when the develop branch is released (as 9.0.beta3).
comment:25 Changed 11 months ago by
Well, okay. But this is just one line with a typo in a very different line. However, I think you're right.
comment:26 Changed 11 months ago by
 Commit changed from c790d130e6d99868425ae59198fbe8be1c09769c to 4c1de40377772f4e390b373013096c117e2f0007
Branch pushed to git repo; I updated commit sha1. New commits:
4c1de40  Merge branch 'develop' into t/28628/tensor_fields__set_restriction_behaviour

comment:27 Changed 11 months ago by
 Status changed from needs_work to needs_review
comment:28 Changed 11 months ago by
 Status changed from needs_review to needs_work
comment:29 followup: ↓ 33 Changed 11 months ago by
Merge still fails. Is it possible to see the conflict files/lines?
comment:30 Changed 11 months ago by
 Branch changed from u/ghDeRhamSource/tensor_fields__set_restriction_behaviour to public/manifolds/set_restriction28628
 Commit changed from 4c1de40377772f4e390b373013096c117e2f0007 to 9299f894a11b02671472d356d51605d1e5277cf3
comment:31 Changed 11 months ago by
 Status changed from needs_work to needs_review
comment:32 Changed 11 months ago by
Yes! Thank you very much! :)
comment:33 in reply to: ↑ 29 Changed 11 months ago by
Replying to ghDeRhamSource:
Merge still fails. Is it possible to see the conflict files/lines?
Sorry I haven't seen your message before pushing the new branch; it seems we were working on solving the conflict at the same time ;).
The general method I am using to see the conflicting files/lines is the following one:
 start from a clean develop branch (i.e. 9.0.beta3 in the present case)
 do
git checkout b set_restriction28628 git pull trac u/ghDeRhamSource/tensor_fields__set_restriction_behaviour
This generates an error message, stating that there is a conflict in tensorfield.py
.
Edit the latter to fix the conflict and do git add tensorfield.py
, then git commit
, etc.
comment:34 followup: ↓ 35 Changed 11 months ago by
That's a good hint for the future. I just thought, you were fast! :D
However, you solved the merge conflict and I'd say, we take your branch, now.
comment:35 in reply to: ↑ 34 Changed 11 months ago by
Replying to ghDeRhamSource:
That's a good hint for the future.
It seems there is a merge conflict between 9.0.beta3 and #28564; so if you want to practice...
comment:36 Changed 11 months ago by
 Status changed from needs_review to positive_review
The last patchbot is full green.
comment:37 Changed 11 months ago by
 Branch changed from public/manifolds/set_restriction28628 to 9299f894a11b02671472d356d51605d1e5277cf3
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
'set_restriction' modified and 'copy_from' method added