Opened 10 months ago

Closed 8 months ago

#28519 closed task (worksforme)

Metaticket - Tensor Fields, Scalar Fields and Mixed Forms

Reported by: gh-DeRhamSource Owned by:
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: geometry Keywords: manifolds, tensor fields, mixed forms, scalar fields
Cc: tscrim, egourgoulhon Merged in:
Authors: Michael Jung Reviewers:
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #28554, #28562, #28564, #28579, #28578 Stopgaps:

Description (last modified by 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

Change History (97)

comment:1 Changed 10 months ago by gh-DeRhamSource

  • Authors set to Michael Jung
  • Cc tscrim egourgoulhon added
  • 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 10 months ago by gh-DeRhamSource

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

comment:3 Changed 10 months ago by gh-DeRhamSource

  • Description modified (diff)

comment:4 Changed 10 months ago by gh-DeRhamSource

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

comment:5 Changed 10 months ago by gh-DeRhamSource

  • Description modified (diff)

comment:6 Changed 10 months ago by gh-DeRhamSource

  • Branch set to u/gh-DeRhamSource/mixed_forms_improvements

comment:7 Changed 10 months ago by gh-DeRhamSource

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

New commits:

786c322Mixed Form Improvements

comment:8 Changed 10 months ago by gh-DeRhamSource

  • Description modified (diff)

comment:9 Changed 10 months ago by gh-DeRhamSource

  • Description modified (diff)

comment:10 Changed 10 months ago by gh-DeRhamSource

  • Description modified (diff)

comment:11 Changed 10 months ago by gh-DeRhamSource

  • Description modified (diff)

comment:12 Changed 10 months ago by git

  • Commit changed from 786c32297948142bc75ba0927a41d2520a634576 to cfba1a2983e3a8319926eaa417200422c8d3499c

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

cfba1a2Built fixes and cosmetic details

comment:13 Changed 10 months ago by gh-DeRhamSource

  • Reviewers set to Eric Gourgoulhon, Travis Scrimshaw

comment:14 Changed 10 months ago by git

  • Commit changed from cfba1a2983e3a8319926eaa417200422c8d3499c to a403ca6ee12b5dafb0935bfe4a8e3cc9c0081682

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

a403ca6Fixed mistakes

comment:15 Changed 10 months ago by git

  • Commit changed from a403ca6ee12b5dafb0935bfe4a8e3cc9c0081682 to d96b3c4ddd4320b25871e83dc10a9a53edf1ae78

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

d96b3c4set_name method in MixedForm

comment:16 Changed 10 months ago by git

  • Commit changed from d96b3c4ddd4320b25871e83dc10a9a53edf1ae78 to fd036e43e574ed7514a58ebe27e3dc25c7da43de

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

fd036e4division by zero checked via is_trivial_zero for scalar fields

comment:17 Changed 10 months ago by git

  • Commit changed from fd036e43e574ed7514a58ebe27e3dc25c7da43de to 20f5ebf8dea618583e0778373acf0cd26fff063b

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

20f5ebfminor doctest fix

comment:18 Changed 10 months ago by git

  • Commit changed from 20f5ebf8dea618583e0778373acf0cd26fff063b to d4819b7fab58c740bc2934d1810c21cdde239837

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

d4819b7whitespace in doctest for scalar fields

comment:19 Changed 10 months ago by git

  • Commit changed from d4819b7fab58c740bc2934d1810c21cdde239837 to 706437f4fe4abaed3c282d380e41388e8891cb70

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

706437fWhitespace

comment:20 Changed 10 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 10 months ago by git

  • Commit set to a2a42e4a9062dce876b3e154c461f8b5d07444d7

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

a2a42e4Mixed Form Improvements

comment:22 Changed 10 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 10 months ago by git

  • Commit changed from a2a42e4a9062dce876b3e154c461f8b5d07444d7 to cd475911fcd15ca8423dc3ecc6d157e730d2d162

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

a0b7484Redundant code removed + one/zero cases in wedge
cd47591Tensorfield addition simplified in case zero

comment:24 Changed 10 months ago by git

  • Commit changed from cd475911fcd15ca8423dc3ecc6d157e730d2d162 to 7d10da93ff7b239c42d2dcb5d96e81cd9e11971a

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

7d10da9Typo fixed

comment:25 Changed 10 months ago by gh-DeRhamSource

  • Description modified (diff)

comment:26 Changed 10 months ago by gh-DeRhamSource

  • Description modified (diff)

comment:27 Changed 10 months ago by git

  • Commit changed from 7d10da93ff7b239c42d2dcb5d96e81cd9e11971a to a9dab55d91ca1e690ec7032009101889d6f8f70f

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

a9dab55Better copy than sorry

comment:28 Changed 10 months ago by git

  • Commit changed from a9dab55d91ca1e690ec7032009101889d6f8f70f to cf12def3b962118ecc4c3e7d6e2825f0a0b68a43

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

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

comment:29 Changed 10 months ago by gh-DeRhamSource

  • Status changed from needs_review to needs_work

comment:30 Changed 10 months ago by gh-DeRhamSource

  • Description modified (diff)

comment:31 Changed 10 months ago by gh-DeRhamSource

  • Description modified (diff)

comment:32 Changed 10 months ago by git

  • Commit changed from cf12def3b962118ecc4c3e7d6e2825f0a0b68a43 to d16dbd94dfaaa2c07182ac5032d6cd81f50da3b9

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

d16dbd9Scal mul names + '_is_zero' attr added and used

comment:33 Changed 10 months ago by gh-DeRhamSource

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

comment:34 follow-up: Changed 10 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: Changed 10 months ago by 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

comment:36 in reply to: ↑ 35 ; follow-ups: Changed 10 months ago by 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.

comment:37 in reply to: ↑ 36 ; follow-up: Changed 10 months ago by 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 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 10 months ago by gh-DeRhamSource (previous) (diff)

comment:38 in reply to: ↑ 36 ; follow-up: Changed 10 months ago by 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.

comment:39 in reply to: ↑ 38 ; follow-up: Changed 10 months ago by 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?

comment:40 in reply to: ↑ 39 Changed 10 months ago by 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.

comment:41 Changed 10 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: Changed 10 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: Changed 10 months ago by 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?

comment:44 in reply to: ↑ 43 Changed 9 months ago by 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?

comment:45 in reply to: ↑ 43 ; follow-up: Changed 9 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).

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.

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

comment:46 in reply to: ↑ 37 ; follow-up: Changed 9 months ago by 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.

comment:47 in reply to: ↑ 45 Changed 9 months ago by 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.

comment:48 in reply to: ↑ 46 ; follow-up: Changed 9 months ago by 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.)

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

comment:49 follow-up: Changed 9 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 9 months ago by gh-DeRhamSource (previous) (diff)

comment:50 Changed 9 months ago by gh-DeRhamSource

  • Description modified (diff)

comment:51 Changed 9 months ago by gh-DeRhamSource

  • Description modified (diff)

comment:52 follow-ups: Changed 9 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: Changed 9 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 9 months ago by gh-DeRhamSource (previous) (diff)

comment:54 in reply to: ↑ 48 Changed 9 months ago by 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).

comment:55 in reply to: ↑ 52 Changed 9 months ago by 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

comment:56 in reply to: ↑ 53 Changed 9 months ago by 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.

comment:57 in reply to: ↑ 49 Changed 9 months ago by 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.

comment:58 in reply to: ↑ 52 ; follow-up: Changed 9 months ago by 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.

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

comment:59 Changed 9 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:

c95aceaBetter zero treatment for tensor fields

comment:60 in reply to: ↑ 58 ; follow-up: Changed 9 months ago by 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?

comment:61 in reply to: ↑ 60 ; follow-up: Changed 9 months ago by 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?

comment:62 in reply to: ↑ 61 Changed 9 months ago by 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.

comment:63 follow-up: Changed 9 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: Changed 9 months ago by 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).

comment:65 in reply to: ↑ 64 ; follow-up: Changed 9 months ago by 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?

comment:66 in reply to: ↑ 65 Changed 9 months ago by 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.

comment:67 follow-ups: Changed 9 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?

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

comment:68 in reply to: ↑ 67 Changed 9 months ago by 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?

comment:69 in reply to: ↑ 67 Changed 9 months ago by 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?

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

  • Description modified (diff)

comment:72 Changed 9 months ago by gh-DeRhamSource

  • Description modified (diff)

comment:73 Changed 9 months ago by gh-DeRhamSource

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

comment:74 Changed 9 months ago by gh-DeRhamSource

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

comment:75 Changed 9 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 9 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
5094a3eReturn no copies for scalar field operations
a749092Merge branch 't/28562/better_zero_treatment' into t/28519/mixed_forms_and_tensor_fields_metaticket
99c392cCopy and multiplication names
69586ccMerge branch 't/28564/tensor_fields__consistent_naming' into t/28519/mixed_forms_and_tensor_fields_metaticket

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

  • Description modified (diff)

comment:79 Changed 9 months ago by git

  • Commit changed from 69586cc2d5690bab66cccfb9fc81dfe998af9081 to 7ad0657129f5975a1a8d198325cdc2ed6a402f13

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

b3e0468MixedForm copies apply names
2dc22a1Merge branch 't/28564/tensor_fields__consistent_naming' into t/28519/mixed_forms_and_tensor_fields_metaticket
ca8dd88Modules modified
892050eFreeModuleLinearGroup: one treatment
1b96587Zero treatment for FreeModules
b9c8287AutomorphismField: zero treatment + MixedForm: set_restriction and add_comp_by_continuation
94cf06aMixed forms adapted to zero treatment + zero treatment for scalar fields
d949a13Minor code reduction
5d30100More code reduction
7ad0657Merge branch 't/28562/better_zero_treatment' into t/28519/mixed_forms_and_tensor_fields_metaticket

comment:80 Changed 9 months ago by gh-DeRhamSource

#28562 and #28564 updated.

comment:81 Changed 9 months ago by gh-DeRhamSource

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

comment:82 Changed 9 months ago by gh-DeRhamSource

  • Description modified (diff)

comment:83 Changed 9 months ago by gh-DeRhamSource

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

comment:84 Changed 9 months ago by git

  • Commit changed from 7ad0657129f5975a1a8d198325cdc2ed6a402f13 to 6d8248a6d456f7e23348c22a0f0ad5b2ef1b1c0f

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

371a255Zero treatment for wedge product of free alt forms
1b1ee7fZero/One check added
5bcc7afAlternative solution via unsafe method
2865535Obsolete '**kwargs' removed
728061aMerge branch 'better_zero_treatment_alternative' into HEAD
62dabd4Wedge product compatible with scalar fields
16d849eTypos fixed
3fbad71Merge branch 't/28579/wedge_product_with_scalar_fields' into HEAD
c104fc8Scalar field zero treatment
6d8248aMerge branch 'better_zero_treatment_alternative' into mixed_forms_and_tensor_fields_metaticket

comment:85 Changed 9 months ago by gh-DeRhamSource

#28579 merged, #28562 overwritten.

comment:86 Changed 9 months ago by git

  • Commit changed from 6d8248a6d456f7e23348c22a0f0ad5b2ef1b1c0f to 080c68d4a794289fd8ebf7a0fed07701cda2d7bc

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

efd8e03Mixed form zero treatment
185663bMerge branch 't/28562/better_zero_treatment_alternative' into t/28519/mixed_forms_and_tensor_fields_metaticket
266b1b8Doctest improved + code improvements
3e96a15Warnings deleted: Gets obsolete with #28562
5967ee8Changes in documentation
78a81b2Merge branch 't/28579/wedge_product_with_scalar_fields' into t/28578/mixed_forms_code_improvements
080c68dMerge branch 't/28578/mixed_forms_code_improvements' into t/28519/mixed_forms_and_tensor_fields_metaticket (manually)

comment:87 Changed 9 months ago by gh-DeRhamSource

#28578 updated and (manually) merged.

comment:88 Changed 9 months ago by gh-DeRhamSource

  • Description modified (diff)

comment:89 Changed 9 months ago by gh-DeRhamSource

  • Description modified (diff)

comment:90 Changed 9 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
88c31f9Clear components when 'copy_from' is invoked
672d268Reorganization of restriction deletion
c790d13'copy_from' removed
c3666e8Resolve merge conflict of #28628 in Sage 9.0.beta3
9299f89Copy _is_zero in copy_from
b0eda29Merge branch 'public/manifolds/set_restriction-28628' of git://trac.sagemath.org/sage into t/28519/mixed_forms_and_tensor_fields_metaticket
4c1de40Merge branch 'develop' into t/28628/tensor_fields__set_restriction_behaviour
b29d541Merge branch 'public/manifolds/set_restriction-28628' of git://trac.sagemath.org/sage into t/28628/tensor_fields__set_restriction_behaviour
0107812Merge branch 't/28628/tensor_fields__set_restriction_behaviour' into t/28519/mixed_forms_and_tensor_fields_metaticket

comment:91 Changed 9 months ago by git

  • Commit changed from 01078129f02f457e2afc12ae26564dc8396f7ae1 to ed178166f77481ed35ea91fcca4783e7ffabcedf

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

f85de87Merge branch 'develop' into t/28564/tensor_fields__consistent_naming
3bf9a04Merge branch 't/28564/tensor_fields__consistent_naming' into t/28519/mixed_forms_and_tensor_fields_metaticket
af0394aMerge branch 'develop' into t/28578/mixed_forms_code_improvements
cbb5251Doctests finished
9694d1cSpeed up wedge using 'sum'
afebf2fRedundant zero assignment removed in 'wedge'
f9163e0Merge branch 'develop' into t/28578/mixed_forms_code_improvements
d0b3af6Merge branch 'develop' into t/28578/mixed_forms_code_improvements
f507b59Merge branch 't/28578/mixed_forms_code_improvements' into t/28519/mixed_forms_and_tensor_fields_metaticket
ed17816Merge conflicts solved

comment:92 Changed 9 months ago by gh-DeRhamSource

All updated and merge conflicts solved.

comment:93 Changed 8 months ago by git

  • Commit changed from ed178166f77481ed35ea91fcca4783e7ffabcedf to 818caefdc053c3a3c3178c6076237f2f60dc178b

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

a50555aMerge branch 'develop' into t/28519/mixed_forms_and_tensor_fields_metaticket
eb92e6aDoctest fixed
5b2eee2'copy' with individual naming
38aa7f5Merge branch 'develop' into t/28564/tensor_fields__consistent_naming
9d39cb5Merge branch 'develop' into t/28564/tensor_fields__consistent_naming
67e5cdcTrac #28564: One line doctest error fixed
5dd53b0Trac #28564: raw strings for LaTeX code
818caefMerge branch 't/28564/tensor_fields__consistent_naming' into t/28519/mixed_forms_and_tensor_fields_metaticket

comment:94 follow-up: Changed 8 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 8 months ago by 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".

comment:96 Changed 8 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 8 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.