# Metaticket - Tensor Fields, Scalar Fields and Mixed Forms

Reported by: Owned by: gh-DeRhamSource major sage-duplicate/invalid/wontfix geometry manifolds, tensor fields, mixed forms, scalar fields tscrim, egourgoulhon Michael Jung N/A #28554, #28562, #28564, #28579, #28578

There are too many changes going on at once, so that a merge in the end might be difficult. In order to avoid such difficulties and to keep an overview of things that are happening (for me and others), I use this ticket from now on as a metaticket, where all latest changes are merged together. Cross ticket references are possible.

This metaticket is mainly devoted to the topic of immutability of algebra elements (see discussion below).

This ticket is related to manifolds and therefore part of the permanent metaticket #18528.

Things going on:

• #28554: Scalar Field Restrictions
• #28562: Better Zero Treatment
• #28564: Consistent Naming for Tensor and Scalar Fields
• #28579: Wedge Product with Scalar Fields
• #28578: Mixed Forms Code Improvements
• #28628: Restriction Behaviour of Tensor Fields

### comment:1 Changed 19 months ago by gh-DeRhamSource

• Authors set to Michael Jung
• Component changed from PLEASE CHANGE to geometry
• Description modified (diff)
• Keywords manifolds mixed forms added
• Summary changed from Mixed Forms Improvements to Differential and Mixed Forms Improvements

### comment:2 Changed 19 months ago by gh-DeRhamSource

• Description modified (diff)
• Type changed from PLEASE CHANGE to enhancement

### comment:3 Changed 19 months ago by gh-DeRhamSource

• Description modified (diff)

### comment:4 Changed 19 months ago by gh-DeRhamSource

Opinions and feedback to the upcoming changes are welcome. :)

### comment:5 Changed 19 months ago by gh-DeRhamSource

• Description modified (diff)

### comment:6 Changed 19 months ago by gh-DeRhamSource

• Branch set to u/gh-DeRhamSource/mixed_forms_improvements

### comment:7 Changed 19 months ago by gh-DeRhamSource

• Commit set to 786c32297948142bc75ba0927a41d2520a634576
• Status changed from new to needs_review

New commits:

 ​786c322 Mixed Form Improvements

### comment:8 Changed 19 months ago by gh-DeRhamSource

• Description modified (diff)

### comment:9 Changed 19 months ago by gh-DeRhamSource

• Description modified (diff)

### comment:10 Changed 19 months ago by gh-DeRhamSource

• Description modified (diff)

### comment:11 Changed 19 months ago by gh-DeRhamSource

• Description modified (diff)

### comment:12 Changed 19 months ago by git

• Commit changed from 786c32297948142bc75ba0927a41d2520a634576 to cfba1a2983e3a8319926eaa417200422c8d3499c

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

 ​cfba1a2 Built fixes and cosmetic details

### comment:13 Changed 19 months ago by gh-DeRhamSource

• Reviewers set to Eric Gourgoulhon, Travis Scrimshaw

### comment:14 Changed 19 months ago by git

• Commit changed from cfba1a2983e3a8319926eaa417200422c8d3499c to a403ca6ee12b5dafb0935bfe4a8e3cc9c0081682

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

 ​a403ca6 Fixed mistakes

### comment:15 Changed 19 months ago by git

• Commit changed from a403ca6ee12b5dafb0935bfe4a8e3cc9c0081682 to d96b3c4ddd4320b25871e83dc10a9a53edf1ae78

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

 ​d96b3c4 set_name method in MixedForm

### comment:16 Changed 19 months ago by git

• Commit changed from d96b3c4ddd4320b25871e83dc10a9a53edf1ae78 to fd036e43e574ed7514a58ebe27e3dc25c7da43de

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

 ​fd036e4 division by zero checked via is_trivial_zero for scalar fields

### comment:17 Changed 19 months ago by git

• Commit changed from fd036e43e574ed7514a58ebe27e3dc25c7da43de to 20f5ebf8dea618583e0778373acf0cd26fff063b

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

 ​20f5ebf minor doctest fix

### comment:18 Changed 19 months ago by git

• Commit changed from 20f5ebf8dea618583e0778373acf0cd26fff063b to d4819b7fab58c740bc2934d1810c21cdde239837

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

 ​d4819b7 whitespace in doctest for scalar fields

### comment:19 Changed 19 months ago by git

• Commit changed from d4819b7fab58c740bc2934d1810c21cdde239837 to 706437f4fe4abaed3c282d380e41388e8891cb70

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

 ​706437f Whitespace

### comment:20 Changed 19 months ago by gh-DeRhamSource

• Branch changed from u/gh-DeRhamSource/mixed_forms_improvements to u/gh-DeRhamSource/mixed_forms_improved
• Commit 706437f4fe4abaed3c282d380e41388e8891cb70 deleted

### comment:21 Changed 19 months ago by git

• Commit set to a2a42e4a9062dce876b3e154c461f8b5d07444d7

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

 ​a2a42e4 Mixed Form Improvements

### comment:22 Changed 19 months ago by gh-DeRhamSource

Sorry for the mess. In the new branch, the commits are cleared and squashed.

I'm looking forward to your review. :)

### comment:23 Changed 19 months ago by git

• Commit changed from a2a42e4a9062dce876b3e154c461f8b5d07444d7 to cd475911fcd15ca8423dc3ecc6d157e730d2d162

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

 ​a0b7484 Redundant code removed + one/zero cases in wedge ​cd47591 Tensorfield addition simplified in case zero

### comment:24 Changed 19 months ago by git

• Commit changed from cd475911fcd15ca8423dc3ecc6d157e730d2d162 to 7d10da93ff7b239c42d2dcb5d96e81cd9e11971a

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

 ​7d10da9 Typo fixed

### comment:25 Changed 19 months ago by gh-DeRhamSource

• Description modified (diff)

### comment:26 Changed 19 months ago by gh-DeRhamSource

• Description modified (diff)

### comment:27 Changed 19 months ago by git

• Commit changed from 7d10da93ff7b239c42d2dcb5d96e81cd9e11971a to a9dab55d91ca1e690ec7032009101889d6f8f70f

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

 ​a9dab55 Better copy than sorry

### comment:28 Changed 19 months ago by git

• Commit changed from a9dab55d91ca1e690ec7032009101889d6f8f70f to cf12def3b962118ecc4c3e7d6e2825f0a0b68a43

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

 ​cf12def Better copy than sorry 2 and '__pos__()' / '__neg__()' for mixed forms

### comment:29 Changed 19 months ago by gh-DeRhamSource

• Status changed from needs_review to needs_work

### comment:30 Changed 19 months ago by gh-DeRhamSource

• Description modified (diff)

### comment:31 Changed 19 months ago by gh-DeRhamSource

• Description modified (diff)

### comment:32 Changed 19 months ago by git

• Commit changed from cf12def3b962118ecc4c3e7d6e2825f0a0b68a43 to d16dbd94dfaaa2c07182ac5032d6cd81f50da3b9

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

 ​d16dbd9 Scal mul names + '_is_zero' attr added and used

### comment:33 Changed 19 months ago by gh-DeRhamSource

• Description modified (diff)
• Status changed from needs_work to needs_review

### comment:34 follow-up: ↓ 35 Changed 19 months ago by egourgoulhon

Thanks for these improvements in the code.

A quick question: what is the motivation for returning a copy of the zero element of the parent instead of the zero element itself? Returning the zero element is certainly faster. In which case wouldn't it be safe? Moreover in some cases (scalar fields) the zero element is updated when a new chart is added, which is not the case of the copy.

### comment:35 in reply to: ↑ 34 ; follow-up: ↓ 36 Changed 19 months ago by gh-DeRhamSource

A quick question: what is the motivation for returning a copy of the zero element of the parent instead of the zero element itself? Returning the zero element is certainly faster. In which case wouldn't it be safe?

The method zero() of the parent is cached. It might happen that this zero element could be manipulated after some computation returning zero (due to some user written algorithm). I tried to avoid that. Do you think this scenario is too unlikely? If so, we can give up the copies.

Moreover in some cases (scalar fields) the zero element is updated when a new chart is added, which is not the case of the copy.

Oh, I was not aware of that. However, a result of an algebraic operation has always been a very new instance, which knows only about the current charts and frames. The user should always be aware of that. At this stage, there is no difference to the previous code and it sounds fairly consistent to me.

What do you think, which way is most convenient and user friendly?

Best regards, Michael

### comment:36 in reply to: ↑ 35 ; follow-ups: ↓ 37 ↓ 38 Changed 19 months ago by egourgoulhon

The method zero() of the parent is cached. It might happen that this zero element could be manipulated after some computation returning zero (due to some user written algorithm). I tried to avoid that. Do you think this scenario is too unlikely? If so, we can give up the copies.

...

Oh, I was not aware of that. However, a result of an algebraic operation has always been a very new instance, which knows only about the current charts and frames. The user should always be aware of that. At this stage, there is no difference to the previous code and it sounds fairly consistent to me.

What do you think, which way is most convenient and user friendly?

Another remark: some of the proposed changes regarding scalar fields are actually not improvements but feature changes:

• the method display() without any argument currently displays the scalar field in all the top charts where the expression is known; you propose to change it to enforce the display in the greatest (sub)charts where the expression can be computed, even if they are not top charts. I'm not sure to agree with that: in the current setting, the output of display() is sufficient to fully characterize the scalar field (no loss of information); the extension you propose adds extra computations and results in longer outputs, which can be cumbersome for complicated scalar fields. If the user would like to know the expression of the scalar field in such a subchart, he/her can get it anyway, by passing the chart as the argument of display().
• regarding the multiplication of scalar fields f and g, the LaTeX output is changed from f g to f\cdot g. Is this for aesthetic reasons only? Personaly I would prefer f g, which sounds more standard notation. Maybe the opinion of a third person (Travis?) would be helpful here.

### comment:37 in reply to: ↑ 36 ; follow-up: ↓ 46 Changed 19 months ago by gh-DeRhamSource

Another remark: some of the proposed changes regarding scalar fields are actually not improvements but feature changes:

• the method display() without any argument currently displays the scalar field in all the top charts where the expression is known; you propose to change it to enforce the display in the greatest (sub)charts where the expression can be computed, even if they are not top charts. I'm not sure to agree with that: in the current setting, the output of display() is sufficient to fully characterize the scalar field (no loss of information); the extension you propose adds extra computations and results in longer outputs, which can be cumbersome for complicated scalar fields. If the user would like to know the expression of the scalar field in such a subchart, he/her can get it anyway, by passing the chart as the argument of display().

I tend not to agree. Take for instance the real line with standard "top" chart x and a continuous scalar field f like f(x)=0 for x<-1, f(x)=x+1 for -1<=x<0, f(x)=1-x for 0<=x<1 and f(x)=0 for x>=1. Indeed, this scalar field is continuous. However, it has no single expression in the top chart, but four different expressions in four subcharts. At this stage, the display() command does not present it properly. Therefore, there is a loss of information about the scalar field, which should not happen. Or did I get something wrong?

• regarding the multiplication of scalar fields f and g, the LaTeX output is changed from f g to f\cdot g. Is this for aesthetic reasons only? Personaly I would prefer f g, which sounds more standard notation. Maybe the opinion of a third person (Travis?) would be helpful here.

In general, I agree with you. When I write things down, I don't use the multiplication symbol, too. But in this case (imao), I would prefer it in order to avoid potential misunderstandings in the naming and to clarify multiplications by a scalar field -- especially when formulas get long. In addition, the multiplication symbol is more "formal", you know? But yes, it is just an aesthetical aspect.

Greetings, Michael

Last edited 19 months ago by gh-DeRhamSource (previous) (diff)

### comment:38 in reply to: ↑ 36 ; follow-up: ↓ 39 Changed 19 months ago by tscrim

The method zero() of the parent is cached. It might happen that this zero element could be manipulated after some computation returning zero (due to some user written algorithm). I tried to avoid that. Do you think this scenario is too unlikely? If so, we can give up the copies.

...

Oh, I was not aware of that. However, a result of an algebraic operation has always been a very new instance, which knows only about the current charts and frames. The user should always be aware of that. At this stage, there is no difference to the previous code and it sounds fairly consistent to me.

What do you think, which way is most convenient and user friendly?

In nearly every instance in Sage, elements are considered immutable and (algebraic) operations do produce new elements (but sometimes share information because they are treated as immutable objects and some of their parts do not change with such operations). Really there should not be any functional difference between the result of P.zero() and creating a new zero instance, and the cached object should be better as it should retain more intermediate computations (say, its values on different charts). If a computation is actually changing some fundamental data, then that is a bug. So I strongly believe you should be using P.zero() instead of creating a new zero instance.

### comment:39 in reply to: ↑ 38 ; follow-up: ↓ 40 Changed 19 months ago by gh-DeRhamSource

In nearly every instance in Sage, elements are considered immutable and (algebraic) operations do produce new elements (but sometimes share information because they are treated as immutable objects and some of their parts do not change with such operations). Really there should not be any functional difference between the result of P.zero() and creating a new zero instance, and the cached object should be better as it should retain more intermediate computations (say, its values on different charts). If a computation is actually changing some fundamental data, then that is a bug. So I strongly believe you should be using P.zero() instead of creating a new zero instance.

And what about keeping an instance of zero internally, changing it internally, and returning a copy of it via zero()? Then, the zero is safe, anyway.

However, my thoughts on this were the following: Assume someone is filling up a mixed form with zeroes and uses the set_restriction method, which invokes all set_restriction methods of its components, to get a new form (just for convenience because most of the components of the resulting mixed form shall be zero in the end). Then, this has an effect on the cached zero elements. Because of that, I decided to copy all the components of the mixed form before declaring them. For consistency reasons I changed everything up like it is now.

Do you have a better suggestion for this kind of situation? And how would you manage it: Copies or no copies for mixed forms?

### comment:40 in reply to: ↑ 39 Changed 19 months ago by tscrim

In nearly every instance in Sage, elements are considered immutable and (algebraic) operations do produce new elements (but sometimes share information because they are treated as immutable objects and some of their parts do not change with such operations). Really there should not be any functional difference between the result of P.zero() and creating a new zero instance, and the cached object should be better as it should retain more intermediate computations (say, its values on different charts). If a computation is actually changing some fundamental data, then that is a bug. So I strongly believe you should be using P.zero() instead of creating a new zero instance.

And what about keeping an instance of zero internally, changing it internally, and returning a copy of it via zero()? Then, the zero is safe, anyway.

-1 as that defeats the purpose.

However, my thoughts on this were the following: Assume someone is filling up a mixed form with zeroes and uses the set_restriction method, which invokes all set_restriction methods of its components, to get a new form (just for convenience because most of the components of the resulting mixed form shall be zero in the end). Then, this has an effect on the cached zero elements. Because of that, I decided to copy all the components of the mixed form before declaring them. For consistency reasons I changed everything up like it is now.

If that fundamentally changes the zero from what it is intended to representation, which it sounds like you are saying it does (I am a bit out of my depth mathematically here), then that is a bug with the set_restriction method. This might a little better/easier for me to see with a more explicit example.

Do you have a better suggestion for this kind of situation? And how would you manage it: Copies or no copies for mixed forms?

If there really is no other way out of this with keeping the cached behavior, then you have to manually copy the elements when creating the mixed forms.

### comment:41 Changed 19 months ago by gh-DeRhamSource

Here comes an examples, like it is now (without copies):

sage: S2 = Manifold(2, 'S^2')
sage: U = S2.open_subset('U') ; V = S2.open_subset('V') # complement of the North and South pole, respectively
sage: S2.declare_union(U,V)
sage: stereoN.<x,y> = U.chart() # stereographic coordinates from the North pole
sage: stereoS.<u,v> = V.chart() # stereographic coordinates from the South pole
sage: xy_to_uv = stereoN.transition_map(stereoS,
....:                                   (x/(x^2+y^2), y/(x^2+y^2)),
....:                                   intersection_name='W',
....:                                   restrictions1= x^2+y^2!=0,
....:                                   restrictions2= u^2+v^2!=0)
sage: W = U.intersection(V)
sage: uv_to_xy = xy_to_uv.inverse()
sage: omegaU = U.diff_form(1, name='omega')
sage: omegaU[:] = [2,3]; omegaU.display()
omega = 2 dx + 3 dy
sage: FU = U.mixed_form(name='F')
sage: FU[:] = [1,omegaU,0]
sage: FU.display()
F = (unnamed scalar field)  + omega + zero
sage: F = S2.mixed_form(name='F')
sage: F[:] = [0,0,0]
sage: F.set_restriction(FU)
sage: F.display()
F = zero + zero + zero
sage: F[1].display()
zero = 2 dx + 3 dy
sage: F[1] is S2.diff_form_module(1).zero()
True


But now, I see your point. The restriction is called zero and this indicates that this is the immutable zero element, no matter what. The user should be aware of that.

But then, the set_restriction method is not useful anymore. It was introduced to facilitate things -- setting the restriction of all forms in just one command (for future implementations).

Any ideas how to solve this?

### comment:42 follow-up: ↓ 43 Changed 19 months ago by gh-DeRhamSource

But then, one question arises for me: Due to your argument that all algebraic objects are considered immutable, the addition with zero should not return a copy, then:

        # Special cases:
if self._is_zero:
return other.copy()
if other._is_zero:
return self.copy()


This is a snippet from the previous code of scalarfield.py in the method _add_. This should be changed, right? In addition, returning the very same instance is faster.

### comment:43 in reply to: ↑ 42 ; follow-ups: ↓ 44 ↓ 45 Changed 19 months ago by tscrim

But then, one question arises for me: Due to your argument that all algebraic objects are considered immutable, the addition with zero should not return a copy, then:

        # Special cases:
if self._is_zero:
return other.copy()
if other._is_zero:
return self.copy()


This is a snippet from the previous code of scalarfield.py in the method _add_. This should be changed, right? In addition, returning the very same instance is faster.

Yes, I agree that should be changed to return same instance.

However, I am not sure I understand why set_restriction is not useful anymore. What about if there are other forms?

### comment:44 in reply to: ↑ 43 Changed 19 months ago by gh-DeRhamSource

Yes, I agree that should be changed to return same instance.

I'll change this.

However, I am not sure I understand why set_restriction is not useful anymore. What about if there are other forms?

Once a differential form is established and ready for further use, it shall be considered as immutable, right? When a mixed form is fed by these forms, set_restriction alters the as immutable seen forms, which should not happen due to your consideration.

Perhaps it is better to implement a mixed form as an own independent algebraic object which should be initialized in a similar way as differential forms. The components must be set individually anyway. What do you think?

### comment:45 in reply to: ↑ 43 ; follow-up: ↓ 47 Changed 19 months ago by egourgoulhon

Thanks to both of you for this interesting and constructive discussion. Sorry for the delay in replying (I was AFAK for a while).

But then, one question arises for me: Due to your argument that all algebraic objects are considered immutable, the addition with zero should not return a copy, then:

        # Special cases:
if self._is_zero:
return other.copy()
if other._is_zero:
return self.copy()


This is a snippet from the previous code of scalarfield.py in the method _add_. This should be changed, right? In addition, returning the very same instance is faster.

Yes, I agree that should be changed to return same instance.

The point is that tensor field objects (including scalar fields and differential forms) are not immutable, except for the zero element. They cannot be made immutable because, in general, they cannot be fully defined at their creation: they are defined "on the fly" by adding some piece of information like

sage: s.add_expr(1+x, chart=X)
sage: t[0,1] = 1+x


for a scalar field s and a tensor field t.

Things are different for the zero elements: they are fully defined at their creation and they should be immutable. This is not enforced though, and we have currently the following garbage in / garbage out bugs:

sage: M = Manifold(2, 'M')
sage: X.<x,y> = M.chart()
sage: zv = M.vector_field_module().zero()
sage: zv.display()
zero = 0
sage: zv[0] = 1 + x  # this should not be allowed!
sage: zv.display()
zero = (x + 1) d/dx
sage: zv is M.vector_field_module().zero()  # ouch!
True
sage: zs = M.scalar_field_algebra().zero()
sage: zs.display()
zero: M --> R
(x, y) |--> 0
sage: zs.set_expr(1 + x)  # this should not be allowed!
sage: zs.display()
zero: M --> R
(x, y) |--> x + 1
sage: zs is M.scalar_field_algebra().zero()  # ouch!
True


The natural way to enforce the immutability of the zero element would be to subclass the element class. Unfortunately, this turns out to be impossible in Sage's Parent/Element framework, cf. this sage-devel discussion.

However, I am not sure I understand why set_restriction is not useful anymore. What about if there are other forms?

set_restriction should handle properly the zero case: if some element is a zero element, then it should replace it by an ordinary object which equals to zero and apply the restriction to it, making it no longer equal to zero.

Last edited 19 months ago by egourgoulhon (previous) (diff)

### comment:46 in reply to: ↑ 37 ; follow-up: ↓ 48 Changed 19 months ago by egourgoulhon

Another remark: some of the proposed changes regarding scalar fields are actually not improvements but feature changes:

• the method display() without any argument currently displays the scalar field in all the top charts where the expression is known; you propose to change it to enforce the display in the greatest (sub)charts where the expression can be computed, even if they are not top charts. I'm not sure to agree with that: in the current setting, the output of display() is sufficient to fully characterize the scalar field (no loss of information); the extension you propose adds extra computations and results in longer outputs, which can be cumbersome for complicated scalar fields. If the user would like to know the expression of the scalar field in such a subchart, he/her can get it anyway, by passing the chart as the argument of display().

I tend not to agree. Take for instance the real line with standard "top" chart x and a continuous scalar field f like f(x)=0 for x<-1, f(x)=x+1 for -1<=x<0, f(x)=1-x for 0<=x<1 and f(x)=0 for x>=1. Indeed, this scalar field is continuous. However, it has no single expression in the top chart, but four different expressions in four subcharts. At this stage, the display() command does not present it properly. Therefore, there is a loss of information about the scalar field, which should not happen. Or did I get something wrong?

In the current implementation, your example scalar field should be defined at the level of the top chart, by something like

f = M.scalar_field( unit_step(x + 1)*unit_step(1 - x)*(1 - abs(x)) )


In other words, subcharts are not intended to store key information about piecewise defined functions. Maybe the whole code could be changed to allow for this, since this is certainly more user friendly than the above unit_step operations. But this should be done in a different ticket.

### comment:47 in reply to: ↑ 45 Changed 19 months ago by egourgoulhon

The natural way to enforce the immutability of the zero element would be to subclass the element class. Unfortunately, this turns out to be impossible in Sage's Parent/Element framework, cf. this sage-devel discussion.

### comment:48 in reply to: ↑ 46 ; follow-up: ↓ 54 Changed 19 months ago by gh-DeRhamSource

In the current implementation, your example scalar field should be defined at the level of the top chart, by something like

f = M.scalar_field( unit_step(x + 1)*unit_step(1 - x)*(1 - abs(x)) )


In other words, subcharts are not intended to store key information about piecewise defined functions. Maybe the whole code could be changed to allow for this, since this is certainly more user friendly than the above unit_step operations. But this should be done in a different ticket.

Certainly, a set_restriction method is way more user friendly and (in my opinion) preferrable -- especially, in order to make the behaviour of scalar fields and tensor fields more consistent. Furthermore, one can use add_expr_by_continuation in this case more effectively.

The display method, as it is now in this ticket, can be very slow if the expressions are quite complicated. However, when the user wishes a short computation, he can choose a particular chart in which the caclulation should be done.

The algorithm I wrote starts from the top chart downwards and immediately stops if an expression is found. So, the computation time depends on how small the scalar field is split. Another approach would be, to display only known expressions and compute new expressions only if a particular chart is stated.

I can open a new ticket on this issue if you wish.

(EDIT: I opened a new ticket #28554 regarding this issue.)

Last edited 19 months ago by gh-DeRhamSource (previous) (diff)

### comment:49 follow-up: ↓ 57 Changed 19 months ago by gh-DeRhamSource

For now, I see two ways to go:

1) We accept that forms are not immutable. Then, copies in every corner are my preferability to avoid unwanted effects.

2) We see forms as immutable as soon as all components are set (which seems to make sense).

Either way, we should implement it consistently and make a remark or note in the documentary. What do you say?

Last edited 19 months ago by gh-DeRhamSource (previous) (diff)

### comment:50 Changed 19 months ago by gh-DeRhamSource

• Description modified (diff)

### comment:51 Changed 19 months ago by gh-DeRhamSource

• Description modified (diff)

### comment:52 follow-ups: ↓ 55 ↓ 58 Changed 19 months ago by tscrim

Adding extra information is not changing the element to me, it is still (suppose to be) representing the same, e.g., tensor field but with more information. Enforcing this is a near impossible task IMO, so we just assume good user input (as I think we should), which preserves the element. Since mathematically we have x + 0 = x, a user should not expect that mutating x+0 is different than mutating x.

Also for the immutability of the zero, ever place you do something that mutates it, just add a check for the _is_zero and raise an error if you do mutate it since it is a special element.

### comment:53 follow-up: ↓ 56 Changed 19 months ago by gh-DeRhamSource

• Description modified (diff)

Okay. I'm going to create a new branch. Due to our discussion, treating the components of mixed forms as immutable/fixed -- in the sense of our previous argumentation -- is an appropriate way, in my opinion.

The issues about scalar fields are mainly moved to ticket #28554. I guess, a whole new discussion arises there.

Do you agree?

Last edited 19 months ago by gh-DeRhamSource (previous) (diff)

### comment:54 in reply to: ↑ 48 Changed 19 months ago by egourgoulhon

I can open a new ticket on this issue if you wish.

(EDIT: I opened a new ticket #28554 regarding this issue.)

Very good. I've added it to the metaticket #18528 (btw, feel free to add a link in the metaticket each time you open a ticket related to manifolds).

### comment:55 in reply to: ↑ 52 Changed 19 months ago by egourgoulhon

Adding extra information is not changing the element to me, it is still (suppose to be) representing the same, e.g., tensor field but with more information. Enforcing this is a near impossible task IMO, so we just assume good user input (as I think we should), which preserves the element. Since mathematically we have x + 0 = x, a user should not expect that mutating x+0 is different than mutating x.

+1

Also for the immutability of the zero, ever place you do something that mutates it, just add a check for the _is_zero and raise an error if you do mutate it since it is a special element.

+1

### comment:56 in reply to: ↑ 53 Changed 19 months ago by egourgoulhon

Okay. I'm going to create a new branch. Due to our discussion, treating the components of mixed forms as immutable/fixed -- in the sense of our previous argumentation -- is an appropriate way, in my opinion.

The issues about scalar fields are mainly moved to ticket #28554. I guess, a whole new discussion arises there.

Do you agree?

Yes, it is usually good to split different issues in various tickets.

### comment:57 in reply to: ↑ 49 Changed 19 months ago by egourgoulhon

Either way, we should implement it consistently and make a remark or note in the documentary. What do you say?

+1 for consistency and documentation. It's true that scalar fields and tensor fields (including differential forms) have been implemented in separate stages (cf. #18528) and have been subsequently modified in possible different ways. This results in some inconsistencies in the user interface. For instance:

• tensor fields have a method set_restriction(), while scalar fields do not (as you noticed)
• scalar fields have a fast method is_trivial_zero(), while tensor fields do not

Thanks for having a look into this.

### comment:58 in reply to: ↑ 52 ; follow-up: ↓ 60 Changed 19 months ago by gh-DeRhamSource

Also for the immutability of the zero, ever place you do something that mutates it, just add a check for the _is_zero and raise an error if you do mutate it since it is a special element.

The _is_zero attribute changes whenever a component is changed or bool is used. So, I'd prefer something like

if self is self._parent_zero:
raise AssertionError("...")


Do you agree? I would do the same thing for the one element of scalar fields.

Last edited 19 months ago by gh-DeRhamSource (previous) (diff)

### comment:59 Changed 19 months ago by git

• Commit changed from d16dbd94dfaaa2c07182ac5032d6cd81f50da3b9 to c95acea6b60d6d60d4956b912e61d6189b6f9a18

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

 ​c95acea Better zero treatment for tensor fields

### comment:60 in reply to: ↑ 58 ; follow-up: ↓ 61 Changed 19 months ago by gh-DeRhamSource

Also for the immutability of the zero, ever place you do something that mutates it, just add a check for the _is_zero and raise an error if you do mutate it since it is a special element.

The _is_zero attribute changes whenever a component is changed or bool is used. So, I'd prefer something like

if self is self._parent_zero:
raise AssertionError("...")


Do you agree? I would do the same thing for the one element of scalar fields.

Nah. Certainly, this will not work! The zero element is altered by using the component manipulation methods. This will raise an immediate error. By the way, the same problem will arise when we use the _is_zero attribute and add a new chart.

Maybe, the zero element should be altered internally via the _comp attribute and any invokation of add_comp, set_comp etc. should be forbidden for this element. What do you say?

### comment:61 in reply to: ↑ 60 ; follow-up: ↓ 62 Changed 19 months ago by egourgoulhon

The _is_zero attribute changes whenever a component is changed or bool is used. So, I'd prefer something like

if self is self._parent_zero:
raise AssertionError("...")


Do you agree? I would do the same thing for the one element of scalar fields.

Nah. Certainly, this will not work! The zero element is altered by using the component manipulation methods. This will raise an immediate error. By the way, the same problem will arise when we use the _is_zero attribute and add a new chart.

I would have written

if self is self.parent().zero():
raise AssertionError("...")


Can you be more specific about the fact that this would not work?

### comment:62 in reply to: ↑ 61 Changed 19 months ago by gh-DeRhamSource

I would have written

if self is self.parent().zero():
raise AssertionError("...")


Can you be more specific about the fact that this would not work?

Yes. In tensorfield_module.py the zero element is established by:

        resu = self._element_constructor_(name='zero', latex_name='0')
for frame in self._domain._frames:
if self._dest_map.restrict(frame._domain) == frame._dest_map:
# (since new components are initialized to zero)


But this raises an immediate error when using

if self is self.parent().zero():
raise AssertionError("...")


in add_comp.

One can add the components from "behind". But using this method is contradictional in this case.

### comment:63 follow-up: ↓ 64 Changed 19 months ago by gh-DeRhamSource

Another idea would be, to move the preexisting code to an internal method called _add_comp and change everything internally with it. From outside then, you have to use the old add_comp method which ensures correct treatment and invokes _add_comp after some checking.

### comment:64 in reply to: ↑ 63 ; follow-up: ↓ 65 Changed 19 months ago by egourgoulhon

Thanks for provided example, with add_comp called on the zero element.

Another idea would be, to move the preexisting code to an internal method called _add_comp and change everything internally with it. From outside then, you have to use the old add_comp method which ensures correct treatment and invokes _add_comp after some checking.

Another option would be to add the keyword argument check=True to add_comp and to use it as follows:

def add_comp(self, basis=None, check=True):
if check and self is self.parent().zero():
raise RuntimeError("...")
...


Then in internal code, like in the example of comment:62, one could call add_comp with check=False.

That said, I would propose to move the secure manipulation of the zero element to another ticket, possibly investigating further the option of introducing a specific subclass (this would avoid having if self is self.parent().zero() cases everywhere).

### comment:65 in reply to: ↑ 64 ; follow-up: ↓ 66 Changed 19 months ago by gh-DeRhamSource

Another option would be to add the keyword argument check=True to add_comp and to use it as follows:

def add_comp(self, basis=None, check=True):
if check and self is self.parent().zero():
raise RuntimeError("...")
...


Then in internal code, like in the example of comment:62, one could call add_comp with check=False.

Sounds good. That would cause the fewest changes in the whole code.

That said, I would propose to move the secure manipulation of the zero element to another ticket, possibly investigating further the option of introducing a specific subclass (this would avoid having if self is self.parent().zero() cases everywhere).

Sounds reasonable. Do you open the ticket or should I?

### comment:66 in reply to: ↑ 65 Changed 19 months ago by egourgoulhon

That said, I would propose to move the secure manipulation of the zero element to another ticket, possibly investigating further the option of introducing a specific subclass (this would avoid having if self is self.parent().zero() cases everywhere).

Sounds reasonable. Do you open the ticket or should I?

Please go on, open the ticket.

### comment:67 follow-ups: ↓ 68 ↓ 69 Changed 19 months ago by gh-DeRhamSource

Okay, things become quite unhandy now. All changes and modifications we had talked about belong to the very same concept of immutability, so things are highly coupled and connected. That makes a merge in the end quite difficult. Perhaps, I should use this as a meta ticket, where things will merged together in the end?

Version 0, edited 19 months ago by gh-DeRhamSource (next)

### comment:68 in reply to: ↑ 67 Changed 19 months ago by gh-DeRhamSource

Okay, things become quite unhandy now. All changes and modifications we had talked about belong to the very same concept of immutability, so things are highly coupled and connected. That makes a merge in the end quite difficult. Perhaps, I should use this as a meta ticket, where things will be merged together in the end?

### comment:69 in reply to: ↑ 67 Changed 19 months ago by egourgoulhon

Okay, things become quite unhandy now. All changes and modifications we had talked about belong to the very same concept of immutability, so things are highly coupled and connected. That makes a merge in the end quite difficult. Perhaps, I should use this as a meta ticket, where things will be merged together in the end?

Yes there should be a ticket (or metaticket?) entirely devoted to (im)mutability of scalar and tensor fields on manifolds, including the treatment of the zero elements. This is beyond the scope of the current ticket, which, according to its title, is devoted to improvements in differential and mixed forms. Can you still implement some of these improvements without touching the mutability of tensor fields for the moment?

### comment:70 Changed 19 months ago by gh-DeRhamSource

• Dependencies set to #28554
• Description modified (diff)
• Keywords tensor fields scalar fields added
• Reviewers Eric Gourgoulhon, Travis Scrimshaw deleted
• Status changed from needs_review to needs_work
• Summary changed from Differential and Mixed Forms Improvements to Metaticket - Tensor Fields, Scalar Fields and Mixed Forms

### comment:71 Changed 19 months ago by gh-DeRhamSource

• Description modified (diff)

### comment:72 Changed 19 months ago by gh-DeRhamSource

• Description modified (diff)

### comment:73 Changed 19 months ago by gh-DeRhamSource

• Dependencies changed from #28554 to #28554, #28562

### comment:74 Changed 19 months ago by gh-DeRhamSource

• Dependencies changed from #28554, #28562 to #28554, #28562, #28564
• Description modified (diff)

### comment:75 Changed 19 months ago by gh-DeRhamSource

• Branch changed from u/gh-DeRhamSource/mixed_forms_improved to u/gh-DeRhamSource/mixed_forms_and_tensor_fields_metaticket
• Commit c95acea6b60d6d60d4956b912e61d6189b6f9a18 deleted

### comment:76 Changed 19 months ago by git

• Commit set to 69586cc2d5690bab66cccfb9fc81dfe998af9081

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

 ​1d68a1f 'set_restriction' added + some restriction modifications ​9f91503 '_is_zero' attribute added and modified ​5094a3e Return no copies for scalar field operations ​a749092 Merge branch 't/28562/better_zero_treatment' into t/28519/mixed_forms_and_tensor_fields_metaticket ​99c392c Copy and multiplication names ​69586cc Merge branch 't/28564/tensor_fields__consistent_naming' into t/28519/mixed_forms_and_tensor_fields_metaticket

### comment:77 Changed 19 months ago by gh-DeRhamSource

I hope, the latest changes are now correctly splitted into the tickets and merged together appropriately. Please check.

### comment:78 Changed 19 months ago by gh-DeRhamSource

• Description modified (diff)

### comment:79 Changed 18 months ago by git

• Commit changed from 69586cc2d5690bab66cccfb9fc81dfe998af9081 to 7ad0657129f5975a1a8d198325cdc2ed6a402f13

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

 ​b3e0468 MixedForm copies apply names ​2dc22a1 Merge branch 't/28564/tensor_fields__consistent_naming' into t/28519/mixed_forms_and_tensor_fields_metaticket ​ca8dd88 Modules modified ​892050e FreeModuleLinearGroup: one treatment ​1b96587 Zero treatment for FreeModules ​b9c8287 AutomorphismField: zero treatment + MixedForm: set_restriction and add_comp_by_continuation ​94cf06a Mixed forms adapted to zero treatment + zero treatment for scalar fields ​d949a13 Minor code reduction ​5d30100 More code reduction ​7ad0657 Merge branch 't/28562/better_zero_treatment' into t/28519/mixed_forms_and_tensor_fields_metaticket

### comment:80 Changed 18 months ago by gh-DeRhamSource

#28562 and #28564 updated.

### comment:81 Changed 18 months ago by gh-DeRhamSource

• Dependencies changed from #28554, #28562, #28564 to #28554, #28562, #28564, #28579
• Description modified (diff)

### comment:82 Changed 18 months ago by gh-DeRhamSource

• Description modified (diff)

### comment:83 Changed 18 months ago by gh-DeRhamSource

• Dependencies changed from #28554, #28562, #28564, #28579 to #28554, #28562, #28564, #28579, #28578

### comment:84 Changed 18 months ago by git

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

 ​371a255 Zero treatment for wedge product of free alt forms ​1b1ee7f Zero/One check added ​5bcc7af Alternative solution via unsafe method ​2865535 Obsolete '**kwargs' removed ​728061a Merge branch 'better_zero_treatment_alternative' into HEAD ​62dabd4 Wedge product compatible with scalar fields ​16d849e Typos fixed ​3fbad71 Merge branch 't/28579/wedge_product_with_scalar_fields' into HEAD ​c104fc8 Scalar field zero treatment ​6d8248a Merge branch 'better_zero_treatment_alternative' into mixed_forms_and_tensor_fields_metaticket

### comment:85 Changed 18 months ago by gh-DeRhamSource

#28579 merged, #28562 overwritten.

### comment:86 Changed 18 months ago by git

• Commit changed from 6d8248a6d456f7e23348c22a0f0ad5b2ef1b1c0f to 080c68d4a794289fd8ebf7a0fed07701cda2d7bc

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

 ​efd8e03 Mixed form zero treatment ​185663b Merge branch 't/28562/better_zero_treatment_alternative' into t/28519/mixed_forms_and_tensor_fields_metaticket ​266b1b8 Doctest improved + code improvements ​3e96a15 Warnings deleted: Gets obsolete with #28562 ​5967ee8 Changes in documentation ​78a81b2 Merge branch 't/28579/wedge_product_with_scalar_fields' into t/28578/mixed_forms_code_improvements ​080c68d Merge branch 't/28578/mixed_forms_code_improvements' into t/28519/mixed_forms_and_tensor_fields_metaticket (manually)

### comment:87 Changed 18 months ago by gh-DeRhamSource

#28578 updated and (manually) merged.

### comment:88 Changed 18 months ago by gh-DeRhamSource

• Description modified (diff)

### comment:89 Changed 18 months ago by gh-DeRhamSource

• Description modified (diff)

### comment:90 Changed 18 months ago by git

• Commit changed from 080c68d4a794289fd8ebf7a0fed07701cda2d7bc to 01078129f02f457e2afc12ae26564dc8396f7ae1

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

 ​e30ec5b 'set_restriction' modified and 'copy_from' method added ​88c31f9 Clear components when 'copy_from' is invoked ​672d268 Reorganization of restriction deletion ​c790d13 'copy_from' removed ​c3666e8 Resolve merge conflict of #28628 in Sage 9.0.beta3 ​9299f89 Copy _is_zero in copy_from ​b0eda29 Merge branch 'public/manifolds/set_restriction-28628' of git://trac.sagemath.org/sage into t/28519/mixed_forms_and_tensor_fields_metaticket ​4c1de40 Merge branch 'develop' into t/28628/tensor_fields__set_restriction_behaviour ​b29d541 Merge branch 'public/manifolds/set_restriction-28628' of git://trac.sagemath.org/sage into t/28628/tensor_fields__set_restriction_behaviour ​0107812 Merge branch 't/28628/tensor_fields__set_restriction_behaviour' into t/28519/mixed_forms_and_tensor_fields_metaticket

### comment:91 Changed 18 months ago by git

• Commit changed from 01078129f02f457e2afc12ae26564dc8396f7ae1 to ed178166f77481ed35ea91fcca4783e7ffabcedf

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

 ​f85de87 Merge branch 'develop' into t/28564/tensor_fields__consistent_naming ​3bf9a04 Merge branch 't/28564/tensor_fields__consistent_naming' into t/28519/mixed_forms_and_tensor_fields_metaticket ​af0394a Merge branch 'develop' into t/28578/mixed_forms_code_improvements ​cbb5251 Doctests finished ​9694d1c Speed up wedge using 'sum' ​afebf2f Redundant zero assignment removed in 'wedge' ​f9163e0 Merge branch 'develop' into t/28578/mixed_forms_code_improvements ​d0b3af6 Merge branch 'develop' into t/28578/mixed_forms_code_improvements ​f507b59 Merge branch 't/28578/mixed_forms_code_improvements' into t/28519/mixed_forms_and_tensor_fields_metaticket ​ed17816 Merge conflicts solved

### comment:92 Changed 18 months ago by gh-DeRhamSource

All updated and merge conflicts solved.

### comment:93 Changed 17 months ago by git

• Commit changed from ed178166f77481ed35ea91fcca4783e7ffabcedf to 818caefdc053c3a3c3178c6076237f2f60dc178b

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

 ​a50555a Merge branch 'develop' into t/28519/mixed_forms_and_tensor_fields_metaticket ​eb92e6a Doctest fixed ​5b2eee2 'copy' with individual naming ​38aa7f5 Merge branch 'develop' into t/28564/tensor_fields__consistent_naming ​9d39cb5 Merge branch 'develop' into t/28564/tensor_fields__consistent_naming ​67e5cdc Trac #28564: One line doctest error fixed ​5dd53b0 Trac #28564: raw strings for LaTeX code ​818caef Merge branch 't/28564/tensor_fields__consistent_naming' into t/28519/mixed_forms_and_tensor_fields_metaticket

### comment:94 follow-up: ↓ 95 Changed 17 months ago by gh-DeRhamSource

• Status changed from needs_work to positive_review

Since all subtickets are merged, this ticket can be seen as finished and merged as-well. Please close.

### comment:95 in reply to: ↑ 94 Changed 17 months ago by egourgoulhon

Since all subtickets are merged, this ticket can be seen as finished and merged as-well. Please close.

I guess all the code is included in the previously merged tickets, so maybe you should remove the branch attached to the current ticket to avoid any confusion. Also, I think the ticket type should be set to "task" instead of "enhancement".

### comment:96 Changed 17 months ago by gh-DeRhamSource

• Branch u/gh-DeRhamSource/mixed_forms_and_tensor_fields_metaticket deleted
• Commit 818caefdc053c3a3c3178c6076237f2f60dc178b deleted
• Type changed from enhancement to task

### comment:97 Changed 17 months ago by chapoton

• Milestone changed from sage-9.0 to sage-duplicate/invalid/wontfix
• Resolution set to worksforme
• Status changed from positive_review to closed

not sure how to handle this kind of ticket. Closing as "works for me".

Note: See TracTickets for help on using tickets.