Opened 19 months ago
Closed 17 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 )
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 19 months 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 19 months ago by
- Description modified (diff)
- Type changed from PLEASE CHANGE to enhancement
comment:3 Changed 19 months ago by
- Description modified (diff)
comment:4 Changed 19 months ago by
comment:5 Changed 19 months ago by
- Description modified (diff)
comment:6 Changed 19 months ago by
- Branch set to u/gh-DeRhamSource/mixed_forms_improvements
comment:7 Changed 19 months ago by
- Commit set to 786c32297948142bc75ba0927a41d2520a634576
- Status changed from new to needs_review
New commits:
786c322 | Mixed Form Improvements
|
comment:8 Changed 19 months ago by
- Description modified (diff)
comment:9 Changed 19 months ago by
- Description modified (diff)
comment:10 Changed 19 months ago by
- Description modified (diff)
comment:11 Changed 19 months ago by
- Description modified (diff)
comment:12 Changed 19 months 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 19 months ago by
- Reviewers set to Eric Gourgoulhon, Travis Scrimshaw
comment:14 Changed 19 months 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 19 months 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 19 months 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 19 months 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 19 months 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 19 months ago by
- Commit changed from d4819b7fab58c740bc2934d1810c21cdde239837 to 706437f4fe4abaed3c282d380e41388e8891cb70
Branch pushed to git repo; I updated commit sha1. New commits:
706437f | Whitespace
|
comment:20 Changed 19 months ago by
- Branch changed from u/gh-DeRhamSource/mixed_forms_improvements to u/gh-DeRhamSource/mixed_forms_improved
- Commit 706437f4fe4abaed3c282d380e41388e8891cb70 deleted
comment:21 Changed 19 months ago by
- Commit set to a2a42e4a9062dce876b3e154c461f8b5d07444d7
Branch pushed to git repo; I updated commit sha1. New commits:
a2a42e4 | Mixed Form Improvements
|
comment:22 Changed 19 months ago by
Sorry for the mess. In the new branch, the commits are cleared and squashed.
I'm looking forward to your review. :)
comment:23 Changed 19 months ago by
- Commit changed from a2a42e4a9062dce876b3e154c461f8b5d07444d7 to cd475911fcd15ca8423dc3ecc6d157e730d2d162
comment:24 Changed 19 months 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 19 months ago by
- Description modified (diff)
comment:26 Changed 19 months ago by
- Description modified (diff)
comment:27 Changed 19 months 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 19 months 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 19 months ago by
- Status changed from needs_review to needs_work
comment:30 Changed 19 months ago by
- Description modified (diff)
comment:31 Changed 19 months ago by
- Description modified (diff)
comment:32 Changed 19 months 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 19 months ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
comment:34 follow-up: ↓ 35 Changed 19 months 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 ; follow-up: ↓ 36 Changed 19 months 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 ; follow-ups: ↓ 37 ↓ 38 Changed 19 months ago by
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 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 ; follow-up: ↓ 46 Changed 19 months 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)=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
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 clearly distinguish tensor/wedge products and multiplications by a scalar field -- especially when formulas get long. In addition, the multiplication symbol is more "formal", you know? But yes, it is just an aesthetical aspect.
Greetings, Michael
comment:38 in reply to: ↑ 36 ; follow-up: ↓ 39 Changed 19 months ago by
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: ↓ 40 Changed 19 months 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 19 months ago by
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 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 19 months 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 follow-up: ↓ 43 Changed 19 months 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 ; follow-ups: ↓ 44 ↓ 45 Changed 19 months ago by
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 19 months 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 ; follow-up: ↓ 47 Changed 19 months 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 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.
comment:46 in reply to: ↑ 37 ; follow-up: ↓ 48 Changed 19 months ago by
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 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)=1-x 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 19 months 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 sage-devel discussion.
See also this thread, which might be relevant.
comment:48 in reply to: ↑ 46 ; follow-up: ↓ 54 Changed 19 months 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 follow-up: ↓ 57 Changed 19 months 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 19 months ago by
- Description modified (diff)
comment:51 Changed 19 months ago by
- Description modified (diff)
comment:52 follow-ups: ↓ 55 ↓ 58 Changed 19 months 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 follow-up: ↓ 56 Changed 19 months 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 19 months ago by
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 19 months 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 19 months ago by
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 19 months ago by
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: ↓ 60 Changed 19 months 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 19 months 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 ; follow-up: ↓ 61 Changed 19 months ago by
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 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 ; follow-up: ↓ 62 Changed 19 months ago by
Replying to gh-DeRhamSource:
Replying to gh-DeRhamSource:
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 19 months 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 follow-up: ↓ 64 Changed 19 months 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 ; follow-up: ↓ 65 Changed 19 months ago by
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 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 ; follow-up: ↓ 66 Changed 19 months 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 19 months ago by
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: ↓ 68 ↓ 69 Changed 19 months 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 19 months ago by
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 19 months ago by
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 19 months 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 19 months ago by
- Description modified (diff)
comment:72 Changed 19 months ago by
- Description modified (diff)
comment:73 Changed 19 months ago by
- Dependencies changed from #28554 to #28554, #28562
comment:74 Changed 19 months ago by
- Dependencies changed from #28554, #28562 to #28554, #28562, #28564
- Description modified (diff)
comment:75 Changed 19 months ago by
- Branch changed from u/gh-DeRhamSource/mixed_forms_improved to u/gh-DeRhamSource/mixed_forms_and_tensor_fields_metaticket
- Commit c95acea6b60d6d60d4956b912e61d6189b6f9a18 deleted
comment:76 Changed 19 months ago by
- Commit set to 69586cc2d5690bab66cccfb9fc81dfe998af9081
Branch pushed to git repo; I updated commit sha1. New commits:
1d68a1f | 'set_restriction' added + some restriction modifications
|
9f91503 | '_is_zero' attribute added and modified
|
5094a3e | Return no copies for scalar field operations
|
a749092 | Merge branch 't/28562/better_zero_treatment' into t/28519/mixed_forms_and_tensor_fields_metaticket
|
99c392c | Copy and multiplication names
|
69586cc | Merge branch 't/28564/tensor_fields__consistent_naming' into t/28519/mixed_forms_and_tensor_fields_metaticket
|
comment:77 Changed 19 months ago by
I hope, the latest changes are now correctly splitted into the tickets and merged together appropriately. Please check.
comment:78 Changed 19 months ago by
- Description modified (diff)
comment:79 Changed 19 months 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 19 months ago by
comment:81 Changed 19 months ago by
- Dependencies changed from #28554, #28562, #28564 to #28554, #28562, #28564, #28579
- Description modified (diff)
comment:82 Changed 19 months ago by
- Description modified (diff)
comment:83 Changed 19 months ago by
- Dependencies changed from #28554, #28562, #28564, #28579 to #28554, #28562, #28564, #28579, #28578
comment:84 Changed 18 months 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 18 months ago by
comment:86 Changed 18 months 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 18 months ago by
#28578 updated and (manually) merged.
comment:88 Changed 18 months ago by
- Description modified (diff)
comment:89 Changed 18 months ago by
- Description modified (diff)
comment:90 Changed 18 months 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_restriction-28628' of git://trac.sagemath.org/sage into t/28519/mixed_forms_and_tensor_fields_metaticket
|
4c1de40 | Merge branch 'develop' into t/28628/tensor_fields__set_restriction_behaviour
|
b29d541 | Merge branch 'public/manifolds/set_restriction-28628' of git://trac.sagemath.org/sage into t/28628/tensor_fields__set_restriction_behaviour
|
0107812 | Merge branch 't/28628/tensor_fields__set_restriction_behaviour' into t/28519/mixed_forms_and_tensor_fields_metaticket
|
comment:91 Changed 18 months ago by
- Commit changed from 01078129f02f457e2afc12ae26564dc8396f7ae1 to ed178166f77481ed35ea91fcca4783e7ffabcedf
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
f85de87 | Merge branch 'develop' into t/28564/tensor_fields__consistent_naming
|
3bf9a04 | Merge branch 't/28564/tensor_fields__consistent_naming' into t/28519/mixed_forms_and_tensor_fields_metaticket
|
af0394a | Merge branch 'develop' into t/28578/mixed_forms_code_improvements
|
cbb5251 | Doctests finished
|
9694d1c | Speed up wedge using 'sum'
|
afebf2f | Redundant zero assignment removed in 'wedge'
|
f9163e0 | Merge branch 'develop' into t/28578/mixed_forms_code_improvements
|
d0b3af6 | Merge branch 'develop' into t/28578/mixed_forms_code_improvements
|
f507b59 | Merge branch 't/28578/mixed_forms_code_improvements' into t/28519/mixed_forms_and_tensor_fields_metaticket
|
ed17816 | Merge conflicts solved
|
comment:92 Changed 18 months ago by
All updated and merge conflicts solved.
comment:93 Changed 17 months 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 follow-up: ↓ 95 Changed 17 months ago by
- Status changed from needs_work to positive_review
Since all subtickets are merged, this ticket can be seen as finished and merged as-well. Please close.
comment:95 in reply to: ↑ 94 Changed 17 months ago by
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 17 months ago by
- Branch u/gh-DeRhamSource/mixed_forms_and_tensor_fields_metaticket deleted
- Commit 818caefdc053c3a3c3178c6076237f2f60dc178b deleted
- Type changed from enhancement to task
comment:97 Changed 17 months ago by
- 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".
Opinions and feedback to the upcoming changes are welcome. :)