#28628 closed defect (fixed)

Tensor Fields: set_restriction Behaviour

Reported by: gh-DeRhamSource Owned by:
Priority: major Milestone: sage-9.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 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 (37)

comment:1 Changed 14 months ago by gh-DeRhamSource

  • Branch set to u/gh-DeRhamSource/tensor_fields__set_restriction_behaviour

comment:2 Changed 14 months 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 14 months 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 14 months 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 14 months 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 14 months 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 14 months ago by gh-DeRhamSource

  • Description modified (diff)

comment:8 Changed 14 months ago by git

  • Commit changed from 88c31f99d477220b17dea0795deea481b9660f49 to 672d2683ddbde940b528bdeb578d7d3c09892a1f

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

672d268Reorganization of restriction deletion

comment:9 follow-up: Changed 14 months ago by gh-DeRhamSource

  • Status changed from needs_work to needs_review

Is this better?

comment:10 in reply to: ↑ 9 ; follow-up: Changed 14 months ago by egourgoulhon

Replying to gh-DeRhamSource:

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 ; follow-up: Changed 14 months ago by gh-DeRhamSource

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

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 14 months ago by egourgoulhon

Replying to gh-DeRhamSource:

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 14 months ago by gh-DeRhamSource

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 14 months ago by git

  • Commit changed from 672d2683ddbde940b528bdeb578d7d3c09892a1f to c790d130e6d99868425ae59198fbe8be1c09769c

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

c790d13'copy_from' removed

comment:15 follow-up: Changed 14 months ago by gh-DeRhamSource

Done.

comment:16 in reply to: ↑ 15 Changed 14 months ago by egourgoulhon

Replying to gh-DeRhamSource:

Done.

Thanks.

comment:17 Changed 14 months ago by egourgoulhon

  • Keywords tensor added
  • Reviewers set to Eric Gourgoulhon
  • Status changed from needs_review to positive_review

comment:18 Changed 14 months ago by chapoton

author name missing

comment:19 Changed 14 months ago by vbraun

  • Status changed from positive_review to needs_work

comment:20 Changed 14 months ago by gh-DeRhamSource

  • Authors set to Michael Jung
  • Status changed from needs_work to positive_review

comment:21 follow-up: Changed 14 months ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict

comment:22 in reply to: ↑ 21 Changed 14 months ago by egourgoulhon

Replying to vbraun:

Merge conflict

I guess this is a conflict with one of the recently merged manifold tickets, #28600 or #28159 (more probably the latter). The easiest way to solve it would be to wait for the 9.0.beta3 release...

comment:23 follow-up: Changed 14 months ago by gh-DeRhamSource

No, the affected files are untouched in #28600 and #28159. I rather think it's connected to #28562?

Which code lines cause a merge conflict anyway?

comment:24 in reply to: ↑ 23 Changed 14 months ago by egourgoulhon

Replying to gh-DeRhamSource:

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 14 months ago by gh-DeRhamSource

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 13 months ago by git

  • Commit changed from c790d130e6d99868425ae59198fbe8be1c09769c to 4c1de40377772f4e390b373013096c117e2f0007

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

4c1de40Merge branch 'develop' into t/28628/tensor_fields__set_restriction_behaviour

comment:27 Changed 13 months ago by gh-DeRhamSource

  • Status changed from needs_work to needs_review

comment:28 Changed 13 months ago by gh-DeRhamSource

  • Status changed from needs_review to needs_work

comment:29 follow-up: Changed 13 months ago by gh-DeRhamSource

Merge still fails. Is it possible to see the conflict files/lines?

comment:30 Changed 13 months ago by egourgoulhon

  • Branch changed from u/gh-DeRhamSource/tensor_fields__set_restriction_behaviour to public/manifolds/set_restriction-28628
  • Commit changed from 4c1de40377772f4e390b373013096c117e2f0007 to 9299f894a11b02671472d356d51605d1e5277cf3

The merge conflict with Sage 9.0.beta3 is solved in the above branch. I have also added the copy of the attribute _is_zero in copy_from(), as well as some autorship lines. Do you agree?


New commits:

c3666e8Resolve merge conflict of #28628 in Sage 9.0.beta3
9299f89Copy _is_zero in copy_from

comment:31 Changed 13 months ago by egourgoulhon

  • Status changed from needs_work to needs_review

comment:32 Changed 13 months ago by gh-DeRhamSource

Yes! Thank you very much! :)

comment:33 in reply to: ↑ 29 Changed 13 months ago by egourgoulhon

Replying to gh-DeRhamSource:

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_restriction-28628
    git pull trac u/gh-DeRhamSource/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 follow-up: Changed 13 months ago by gh-DeRhamSource

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 13 months ago by egourgoulhon

Replying to gh-DeRhamSource:

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 13 months ago by egourgoulhon

  • Status changed from needs_review to positive_review

The last patchbot is full green.

comment:37 Changed 13 months ago by vbraun

  • Branch changed from public/manifolds/set_restriction-28628 to 9299f894a11b02671472d356d51605d1e5277cf3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.