Opened 7 months ago

Closed 6 months ago

#29244 closed enhancement (fixed)

Apply a function to all components of a tensor field

Reported by: egourgoulhon Owned by:
Priority: major Milestone: sage-9.1
Component: geometry Keywords: manifolds, tensor, components, sd107
Cc: tscrim Merged in:
Authors: Eric Gourgoulhon Reviewers: Vincent Delecroix, Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 5cf7fc5 (Commits) Commit: 5cf7fc53f523cb3a1a7c8c75df8b2f0774203df1
Dependencies: Stopgaps:

Description (last modified by egourgoulhon)

This ticket adds the method apply_map to the class TensorField, which applies a function to the coordinate expressions of all components of a tensor field in a given vector frame. This allows operations like factorization, expansion, simplification or substitution to be performed on the components.

Such a method has been requested in e.g. https://ask.sagemath.org/question/42107/basic-work-on-tensor-components/ and was discussed again during Sage Days 107.

Change History (20)

comment:1 Changed 7 months ago by egourgoulhon

  • Branch set to public/manifolds/map_tensor_components-29244
  • Commit set to 14937a88e33fee39ec4a6873ee55808b2165ea17

New commits:

14937a8Add method map_components to class TensorField

comment:2 Changed 7 months ago by egourgoulhon

  • Cc tscrim added
  • Status changed from new to needs_review

comment:3 follow-up: Changed 7 months ago by vdelecroix

  • Status changed from needs_review to needs_info

For vectors and matrices, such method already exists and is called apply_map

sage: v = vector(range(4))
sage: v.apply_map(lambda x: x^2)
(0, 1, 4, 9)
sage: m = matrix(3,range(9))
sage: m.apply_map(lambda x: x^2)
[ 0  1  4]
[ 9 16 25]
[36 49 64]

Is the difference in terminology intended?

comment:4 in reply to: ↑ 3 ; follow-up: Changed 7 months ago by egourgoulhon

Replying to vdelecroix:

For vectors and matrices, such method already exists and is called apply_map

Is the difference in terminology intended?

No, not at all. Thanks for pointing out this. In the context of tensor fields, apply_map may sound a little bit too vague (for instance, it may evoke a pull back operation associated with a map between manifolds). Would apply_map_components be more appropriate?

comment:5 in reply to: ↑ 4 ; follow-up: Changed 7 months ago by vdelecroix

Replying to egourgoulhon:

Replying to vdelecroix:

For vectors and matrices, such method already exists and is called apply_map

Is the difference in terminology intended?

No, not at all. Thanks for pointing out this. In the context of tensor fields, apply_map may sound a little bit too vague (for instance, it may evoke a pull back operation associated with a map between manifolds). Would apply_map_components be more appropriate?

The vagueness also applies to vectors/matrices where map can also refer to a morphism between vector spaces. I am not a big fan of the terminology but to my mind, consistency is the best option for the Sage interface as a whole. This consistency didcatorship includes as an option changing apply_map to something better.

comment:6 in reply to: ↑ 5 Changed 7 months ago by vdelecroix

Replying to vdelecroix:

Replying to egourgoulhon:

Replying to vdelecroix:

For vectors and matrices, such method already exists and is called apply_map

Is the difference in terminology intended?

No, not at all. Thanks for pointing out this. In the context of tensor fields, apply_map may sound a little bit too vague (for instance, it may evoke a pull back operation associated with a map between manifolds). Would apply_map_components be more appropriate?

The vagueness also applies to vectors/matrices where map can also refer to a morphism between vector spaces. I am not a big fan of the terminology but to my mind, consistency is the best option for the Sage interface as a whole. This consistency didcatorship includes as an option changing apply_map to something better.

apply_map is redundant. I would just have called it map because of the already existing Python function

sage: list(map(lambda x: x^2, range(4)))
[0, 1, 4, 9]

comment:7 Changed 7 months ago by tscrim

I am -1 on calling the method map as I would first think it should return a map either from or based on the object or a map form of the object (such as for a matrix).

comment:8 follow-up: Changed 7 months ago by egourgoulhon

Thank you Vincent and Travis for the discussion. I am pretty sensitive to the consistency argument put forward in comment:5, so I would lean toward apply_map. Do we all agree?

comment:9 in reply to: ↑ 8 Changed 7 months ago by vdelecroix

Replying to egourgoulhon:

Thank you Vincent and Travis for the discussion. I am pretty sensitive to the consistency argument put forward in comment:5, so I would lean toward apply_map. Do we all agree?

Dear Eric, that is of course ok for me!

comment:10 Changed 7 months ago by gh-mwageringel

I just want to mention that matrices also have a method map_coefficients which is very similar, coming from modules with basis. It is supposed to work with an endofunction on the coefficient ring, however it seems to be broken for matrices:

sage: matrix.identity(2).map_coefficients(lambda x: x+1)
...
KeyError: 1

As apply_map does not require an endofunction, it has a more general notion, though.

comment:11 follow-up: Changed 7 months ago by gh-PatrickMassot

I'm the one who asked for this function, so I'm very happy to see this patch. But I'm uneasy about the way it can be used to introduce inconsistencies between frames. Was it already easy to abuse SageManifolds? like this before?

At least I would phrase that as a warning in the docstrings, instead of making it sound like a feature. I mean, it can be a feature where the function being mapped is non-destructive like factor, but substituting a parameter in some frame but not the other is clearly evil.

comment:12 in reply to: ↑ 11 Changed 7 months ago by egourgoulhon

Replying to gh-PatrickMassot:

I'm the one who asked for this function, so I'm very happy to see this patch. But I'm uneasy about the way it can be used to introduce inconsistencies between frames. Was it already easy to abuse SageManifolds? like this before?

At least I would phrase that as a warning in the docstrings, instead of making it sound like a feature. I mean, it can be a feature where the function being mapped is non-destructive like factor, but substituting a parameter in some frame but not the other is clearly evil.

That's a good point. It should be easy to add a keyword argument such as keep_other_components=False or non-destructive=False to enforce the consistency between the sets of components in various frames. The components in frames different from the requested one will then be deleted. When requested, they can be recomputed via change-of-frame formulas. However, this can be time consuming and, in some cases, may generate a loss of information due to some lack of simplifications.

comment:13 Changed 7 months ago by git

  • Commit changed from 14937a88e33fee39ec4a6873ee55808b2165ea17 to 5cf7fc53f523cb3a1a7c8c75df8b2f0774203df1

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

5cf7fc5#29244: rename map_components to apply_map and add keyword argument keep_other_components

comment:14 Changed 7 months ago by egourgoulhon

In the above commit, the method has been renamed apply_map (cf. comment:9) and the consistency issue pointed out in comment:11 has been delt with by adding the keyword argument keep_other_components. Its default value (False) enforces the consistency among the components in various vector frames.

comment:15 Changed 7 months ago by egourgoulhon

  • Status changed from needs_info to needs_review

comment:16 Changed 7 months ago by egourgoulhon

  • Description modified (diff)

comment:17 Changed 6 months ago by egourgoulhon

Anybody to perform the final review? If we want this in 9.1, this could be the good time...

comment:18 Changed 6 months ago by tscrim

  • Reviewers set to Vincent Delecroix, Travis Scrimshaw
  • Status changed from needs_review to positive_review

I will give this the green light. I added Vincent as a reviewer considering the name change was based off his suggestion. If anyone else feels like they want to be a reviewer, please add your name.

comment:19 Changed 6 months ago by egourgoulhon

Thank you Travis!

comment:20 Changed 6 months ago by vbraun

  • Branch changed from public/manifolds/map_tensor_components-29244 to 5cf7fc53f523cb3a1a7c8c75df8b2f0774203df1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.