Opened 2 years ago

Last modified 2 years ago

#28628 closed defect

Tensor Fields: set_restriction Behaviour — at Version 7

Reported by: gh-DeRhamSource Owned by:
Priority: major Milestone: sage-9.0
Component: geometry Keywords: manifolds, tensor
Cc: egourgoulhon Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: u/gh-DeRhamSource/tensor_fields__set_restriction_behaviour (Commits, GitHub, GitLab) Commit: 88c31f99d477220b17dea0795deea481b9660f49
Dependencies: Stopgaps:

Status badges

Description (last modified by gh-DeRhamSource)

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 (7)

comment:1 Changed 2 years ago by gh-DeRhamSource

  • Branch set to u/gh-DeRhamSource/tensor_fields__set_restriction_behaviour

comment:2 Changed 2 years ago by gh-DeRhamSource

  • 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

New commits:

e30ec5b'set_restriction' modified and 'copy_from' method added

comment:3 Changed 2 years ago by git

  • Commit changed from e30ec5b88f8c3144c08e731d85c4c79e58595cc4 to 88c31f99d477220b17dea0795deea481b9660f49

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

88c31f9Clear components when 'copy_from' is invoked

comment:4 Changed 2 years ago by egourgoulhon

  • 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 2 years ago by gh-DeRhamSource

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 2 years ago by gh-DeRhamSource

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 2 years ago by gh-DeRhamSource

  • Description modified (diff)
Note: See TracTickets for help on using tickets.