Opened 2 years ago
Closed 2 years ago
#28519 closed task (worksforme)
Metaticket  Tensor Fields, Scalar Fields and Mixed Forms
Reported by:  ghDeRhamSource  Owned by:  

Priority:  major  Milestone:  sageduplicate/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 )
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:
Change History (97)
comment:1 Changed 2 years ago by
 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 2 years ago by
 Description modified (diff)
 Type changed from PLEASE CHANGE to enhancement
comment:3 Changed 2 years ago by
 Description modified (diff)
comment:4 Changed 2 years ago by
comment:5 Changed 2 years ago by
 Description modified (diff)
comment:6 Changed 2 years ago by
 Branch set to u/ghDeRhamSource/mixed_forms_improvements
comment:7 Changed 2 years ago by
 Commit set to 786c32297948142bc75ba0927a41d2520a634576
 Status changed from new to needs_review
New commits:
786c322  Mixed Form Improvements

comment:8 Changed 2 years ago by
 Description modified (diff)
comment:9 Changed 2 years ago by
 Description modified (diff)
comment:10 Changed 2 years ago by
 Description modified (diff)
comment:11 Changed 2 years ago by
 Description modified (diff)
comment:12 Changed 2 years ago by
 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 2 years ago by
 Reviewers set to Eric Gourgoulhon, Travis Scrimshaw
comment:14 Changed 2 years ago by
 Commit changed from cfba1a2983e3a8319926eaa417200422c8d3499c to a403ca6ee12b5dafb0935bfe4a8e3cc9c0081682
Branch pushed to git repo; I updated commit sha1. New commits:
a403ca6  Fixed mistakes

comment:15 Changed 2 years ago by
 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 2 years ago by
 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 2 years ago by
 Commit changed from fd036e43e574ed7514a58ebe27e3dc25c7da43de to 20f5ebf8dea618583e0778373acf0cd26fff063b
Branch pushed to git repo; I updated commit sha1. New commits:
20f5ebf  minor doctest fix

comment:18 Changed 2 years ago by
 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 2 years ago by
 Commit changed from d4819b7fab58c740bc2934d1810c21cdde239837 to 706437f4fe4abaed3c282d380e41388e8891cb70
Branch pushed to git repo; I updated commit sha1. New commits:
706437f  Whitespace

comment:20 Changed 2 years ago by
 Branch changed from u/ghDeRhamSource/mixed_forms_improvements to u/ghDeRhamSource/mixed_forms_improved
 Commit 706437f4fe4abaed3c282d380e41388e8891cb70 deleted
comment:21 Changed 2 years ago by
 Commit set to a2a42e4a9062dce876b3e154c461f8b5d07444d7
Branch pushed to git repo; I updated commit sha1. New commits:
a2a42e4  Mixed Form Improvements

comment:22 Changed 2 years ago by
Sorry for the mess. In the new branch, the commits are cleared and squashed.
I'm looking forward to your review. :)
comment:23 Changed 2 years ago by
 Commit changed from a2a42e4a9062dce876b3e154c461f8b5d07444d7 to cd475911fcd15ca8423dc3ecc6d157e730d2d162
comment:24 Changed 2 years ago by
 Commit changed from cd475911fcd15ca8423dc3ecc6d157e730d2d162 to 7d10da93ff7b239c42d2dcb5d96e81cd9e11971a
Branch pushed to git repo; I updated commit sha1. New commits:
7d10da9  Typo fixed

comment:25 Changed 2 years ago by
 Description modified (diff)
comment:26 Changed 2 years ago by
 Description modified (diff)
comment:27 Changed 2 years ago by
 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 2 years ago by
 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 2 years ago by
 Status changed from needs_review to needs_work
comment:30 Changed 2 years ago by
 Description modified (diff)
comment:31 Changed 2 years ago by
 Description modified (diff)
comment:32 Changed 2 years ago by
 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 2 years ago by
 Description modified (diff)
 Status changed from needs_work to needs_review
comment:34 followup: ↓ 35 Changed 2 years ago by
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 ; followup: ↓ 36 Changed 2 years ago by
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 ; followups: ↓ 37 ↓ 38 Changed 2 years ago by
Replying to ghDeRhamSource:
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 ofdisplay()
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 ofdisplay()
.  regarding the multiplication of scalar fields
f
andg
, the LaTeX output is changed fromf g
tof\cdot g
. Is this for aesthetic reasons only? Personaly I would preferf g
, which sounds more standard notation. Maybe the opinion of a third person (Travis?) would be helpful here.
comment:37 in reply to: ↑ 36 ; followup: ↓ 46 Changed 2 years ago by
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 ofdisplay()
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 ofdisplay()
.
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)=1x 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
andg
, the LaTeX output is changed fromf g
tof\cdot g
. Is this for aesthetic reasons only? Personaly I would preferf 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
comment:38 in reply to: ↑ 36 ; followup: ↓ 39 Changed 2 years ago by
Replying to egourgoulhon:
Replying to ghDeRhamSource:
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 ; followup: ↓ 40 Changed 2 years ago by
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 usingP.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 2 years ago by
Replying to ghDeRhamSource:
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 usingP.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 allset_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 2 years ago by
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 followup: ↓ 43 Changed 2 years ago by
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 ; followups: ↓ 44 ↓ 45 Changed 2 years ago by
Replying to ghDeRhamSource:
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 2 years ago by
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 ; followup: ↓ 47 Changed 2 years ago by
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 ghDeRhamSource:
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 sagedevel 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.
comment:46 in reply to: ↑ 37 ; followup: ↓ 48 Changed 2 years ago by
Replying to ghDeRhamSource:
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 ofdisplay()
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 ofdisplay()
.I tend not to agree. Take for instance the real line with standard "top" chart
x
and a continuous scalar fieldf
likef(x)=0 for x<1
,f(x)=x+1 for 1<=x<0
,f(x)=1x for 0<=x<1
andf(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, thedisplay()
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 2 years ago by
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 sagedevel discussion.
See also this thread, which might be relevant.
comment:48 in reply to: ↑ 46 ; followup: ↓ 54 Changed 2 years ago by
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.)
comment:49 followup: ↓ 57 Changed 2 years ago by
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?
comment:50 Changed 2 years ago by
 Description modified (diff)
comment:51 Changed 2 years ago by
 Description modified (diff)
comment:52 followups: ↓ 55 ↓ 58 Changed 2 years ago by
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 followup: ↓ 56 Changed 2 years ago by
 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?
comment:54 in reply to: ↑ 48 Changed 2 years ago by
Replying to ghDeRhamSource:
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 2 years ago by
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 mutatingx+0
is different than mutatingx
.
+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 2 years ago by
Replying to ghDeRhamSource:
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 2 years ago by
Replying to ghDeRhamSource:
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 ; followup: ↓ 60 Changed 2 years ago by
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.
comment:59 Changed 2 years ago by
 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 ; followup: ↓ 61 Changed 2 years ago by
Replying to ghDeRhamSource:
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 orbool
is used. So, I'd prefer something likeif 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 ; followup: ↓ 62 Changed 2 years ago by
Replying to ghDeRhamSource:
Replying to ghDeRhamSource:
The
_is_zero
attribute changes whenever a component is changed orbool
is used. So, I'd prefer something likeif 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 2 years ago by
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 followup: ↓ 64 Changed 2 years ago by
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 ; followup: ↓ 65 Changed 2 years ago by
Replying to ghDeRhamSource:
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 oldadd_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 ; followup: ↓ 66 Changed 2 years ago by
Replying to egourgoulhon:
Another option would be to add the keyword argument
check=True
toadd_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
withcheck=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 2 years ago by
Replying to ghDeRhamSource:
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 followups: ↓ 68 ↓ 69 Changed 2 years ago by
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:68 in reply to: ↑ 67 Changed 2 years ago by
Replying to ghDeRhamSource:
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 2 years ago by
Replying to ghDeRhamSource:
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 2 years ago by
 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 2 years ago by
 Description modified (diff)
comment:72 Changed 2 years ago by
 Description modified (diff)
comment:73 Changed 2 years ago by
 Dependencies changed from #28554 to #28554, #28562
comment:74 Changed 2 years ago by
 Dependencies changed from #28554, #28562 to #28554, #28562, #28564
 Description modified (diff)
comment:75 Changed 2 years ago by
 Branch changed from u/ghDeRhamSource/mixed_forms_improved to u/ghDeRhamSource/mixed_forms_and_tensor_fields_metaticket
 Commit c95acea6b60d6d60d4956b912e61d6189b6f9a18 deleted
comment:76 Changed 2 years ago by
 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 2 years ago by
I hope, the latest changes are now correctly splitted into the tickets and merged together appropriately. Please check.
comment:78 Changed 2 years ago by
 Description modified (diff)
comment:79 Changed 2 years ago by
 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 2 years ago by
comment:81 Changed 2 years ago by
 Dependencies changed from #28554, #28562, #28564 to #28554, #28562, #28564, #28579
 Description modified (diff)
comment:82 Changed 2 years ago by
 Description modified (diff)
comment:83 Changed 2 years ago by
 Dependencies changed from #28554, #28562, #28564, #28579 to #28554, #28562, #28564, #28579, #28578
comment:84 Changed 2 years ago by
 Commit changed from 7ad0657129f5975a1a8d198325cdc2ed6a402f13 to 6d8248a6d456f7e23348c22a0f0ad5b2ef1b1c0f
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 2 years ago by
comment:86 Changed 2 years ago by
 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 2 years ago by
#28578 updated and (manually) merged.
comment:88 Changed 2 years ago by
 Description modified (diff)
comment:89 Changed 2 years ago by
 Description modified (diff)
comment:90 Changed 2 years ago by
 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_restriction28628' 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_restriction28628' 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 2 years ago by
 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 2 years ago by
All updated and merge conflicts solved.
comment:93 Changed 2 years ago by
 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 followup: ↓ 95 Changed 2 years ago by
 Status changed from needs_work to positive_review
Since all subtickets are merged, this ticket can be seen as finished and merged aswell. Please close.
comment:95 in reply to: ↑ 94 Changed 2 years ago by
Replying to ghDeRhamSource:
Since all subtickets are merged, this ticket can be seen as finished and merged aswell. 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 2 years ago by
 Branch u/ghDeRhamSource/mixed_forms_and_tensor_fields_metaticket deleted
 Commit 818caefdc053c3a3c3178c6076237f2f60dc178b deleted
 Type changed from enhancement to task
comment:97 Changed 2 years ago by
 Milestone changed from sage9.0 to sageduplicate/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".
Opinions and feedback to the upcoming changes are welcome. :)