Metaticket - Tensor Fields, Scalar Fields and Mixed Forms

[gh-DeRhamSource]

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

[gh-DeRhamSource]

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

[gh-DeRhamSource]

New commits:

​786c322

Mixed Form Improvements

[git]

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

​cfba1a2

Built fixes and cosmetic details

[git]

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

​a403ca6

Fixed mistakes

[git]

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

​d96b3c4

set_name method in MixedForm

[git]

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

​fd036e4

division by zero checked via is_trivial_zero for scalar fields

[git]

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

​20f5ebf

minor doctest fix

[git]

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

​d4819b7

whitespace in doctest for scalar fields

[git]

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

​706437f

Whitespace

[git]

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

​a2a42e4

Mixed Form Improvements

[gh-DeRhamSource]

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

I'm looking forward to your review. :)

[git]

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

[git]

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

​7d10da9

Typo fixed

[git]

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

​a9dab55

Better copy than sorry

[git]

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

​cf12def

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

[git]

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

​d16dbd9

Scal mul names + '_is_zero' attr added and used

[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.

[gh-DeRhamSource]

Thanks for your reply.

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

[egourgoulhon]

Replying to gh-DeRhamSource:

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?

The more I think about it, the more I'm inclined to agree with you. Travis, do you have any thought about this?

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.

[gh-DeRhamSource]

Replying to 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?

• 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 clearly distinguish tensor/wedge products and 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

[tscrim]

Replying to egourgoulhon:

Replying to gh-DeRhamSource:

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?

The more I think about it, the more I'm inclined to agree with you. Travis, do you have any thought about this?

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.

[gh-DeRhamSource]

Replying to 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.

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?

[tscrim]

Replying to gh-DeRhamSource:

Replying to 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.

[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?

[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.

[tscrim]

Replying to 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.

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?

[gh-DeRhamSource]

Replying to tscrim:

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?

[egourgoulhon]

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

Replying to tscrim:

Replying to 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.

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.

[egourgoulhon]

Replying to gh-DeRhamSource:

Replying to 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.

[egourgoulhon]

Replying to 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.

See also ​this thread, which might be relevant.

[gh-DeRhamSource]

Replying to egourgoulhon:

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

[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?

[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.

[gh-DeRhamSource]

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?

[egourgoulhon]

Replying to gh-DeRhamSource:

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

[egourgoulhon]

Replying to 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.

+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

[egourgoulhon]

Replying to gh-DeRhamSource:

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.

[egourgoulhon]

Replying to gh-DeRhamSource:

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.

[gh-DeRhamSource]

Replying to tscrim:

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.

[git]

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

​c95acea

Better zero treatment for tensor fields

[gh-DeRhamSource]

Replying to gh-DeRhamSource:

Replying to tscrim:

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?

[egourgoulhon]

Replying to gh-DeRhamSource:

Replying to gh-DeRhamSource:

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?

[gh-DeRhamSource]

Replying to egourgoulhon:

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:
                resu.add_comp(frame)
                # (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.

[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.

[egourgoulhon]

Replying to gh-DeRhamSource:

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

[gh-DeRhamSource]

Replying to egourgoulhon:

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?

[egourgoulhon]

Replying to gh-DeRhamSource:

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.

[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?

[gh-DeRhamSource]

Replying to 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?

[egourgoulhon]

Replying to 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?

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?

[git]

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

[gh-DeRhamSource]

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

[git]

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

[gh-DeRhamSource]

#28562 and #28564 updated.

[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

[gh-DeRhamSource]

#28579 merged, #28562 overwritten.

[git]

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)

[gh-DeRhamSource]

#28578 updated and (manually) merged.

[git]

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

[git]

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

[gh-DeRhamSource]

All updated and merge conflicts solved.

[git]

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

[gh-DeRhamSource]

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

[egourgoulhon]

Replying to gh-DeRhamSource:

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".

[chapoton]

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