Opened 23 months ago
Last modified 3 months ago
#30116 needs_info defect
Replace __eq__ by _richcmp_ for manifolds
Reported by: | gh-mjungmath | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-9.7 |
Component: | manifolds | Keywords: | manifolds, coercion |
Cc: | egourgoulhon, tscrim, mkoeppe | Merged in: | |
Authors: | Michael Jung | Reviewers: | Eric Gourgoulhon, Travis Scrimshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | u/gh-mjungmath/test_comparison_tensorfield (Commits, GitHub, GitLab) | Commit: | 2c06105b9ea437e97c35a936de5a7cebabdd6003 |
Dependencies: | #30112, #30267 | Stopgaps: |
Description (last modified by )
Within the coercion frameworks, two objects are usually compared by using _richcmp_
. Using this, both objects are coerced into an element of a common parent before the comparison is performed. The current implementation uses __eq__
instead and involves no prior coercion.
In this ticket, the method __eq__
is replaced by _richcmp_
as intended by the coercion framework, and __ne__
is removed. This includes the following files:
- differentiable/tensorfield.py
- section.py
- chart_func.py
- scalarfield.py
- continuous_map.py
In result, no manual coercions prior to equality checks are necessary and new coercions don't have to be added separately in the future.
To keep some equality checks function, I had to add the coercion from chart functions to scalar fields (see #30112). Furthermore, to unify the behavior for chart functions and to add this behavior into the documentation, I would like to add the dependence of #30267, too.
Change History (67)
comment:1 Changed 23 months ago by
- Dependencies set to #30112
comment:2 Changed 23 months ago by
- Branch set to u/gh-mjungmath/test_comparison_tensorfield
comment:3 Changed 23 months ago by
- Commit set to 6c28c72b3ffb2ceb34b76f007b47b84713d4d0bb
- Dependencies changed from #30112 to #30112, #30108
comment:4 Changed 23 months ago by
- Commit changed from 6c28c72b3ffb2ceb34b76f007b47b84713d4d0bb to 979c815a40b1c96ab8ed2e806afd95e8d5078056
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
979c815 | Trac #30116: __eq__ replaced by _richcmp_
|
comment:5 Changed 23 months ago by
- Dependencies changed from #30112, #30108 to #30112
comment:6 Changed 23 months ago by
- Commit changed from 979c815a40b1c96ab8ed2e806afd95e8d5078056 to 5fcb2e7725cf05fd7243411f1fa98a6aabee578a
Branch pushed to git repo; I updated commit sha1. New commits:
5fcb2e7 | Trac #30116: input info op added
|
comment:7 Changed 23 months ago by
- Status changed from new to needs_review
comment:8 Changed 23 months ago by
- Status changed from needs_review to needs_work
comment:9 Changed 23 months ago by
- Commit changed from 5fcb2e7725cf05fd7243411f1fa98a6aabee578a to 00e647d8c7c877bd1acb53e612ef0ecfda3150ad
comment:10 Changed 23 months ago by
- Commit changed from 00e647d8c7c877bd1acb53e612ef0ecfda3150ad to 86a5438f8f82f718e96b60f3a6f4886bdd176114
comment:11 Changed 23 months ago by
- Status changed from needs_work to needs_info
comment:12 Changed 23 months ago by
- Commit changed from 86a5438f8f82f718e96b60f3a6f4886bdd176114 to 03019c1601c3169932c0fff5a0abcfeaee2a3d71
Branch pushed to git repo; I updated commit sha1. New commits:
03019c1 | Trac #30116: unused import removed + replacement for sections
|
comment:13 Changed 23 months ago by
- Status changed from needs_info to needs_review
comment:14 follow-up: ↓ 25 Changed 23 months ago by
- Status changed from needs_review to needs_info
I am sorry. I am too impatient. Anyway, I get the following error during the doctest:
File "src/sage/manifolds/section.py", line 2401, in sage.manifolds.section.TrivialSection.restrict Failed example: s_D[[1]] == s[[1]] Expected: False Got: True
Here is the complete related doctest:
sage: M = Manifold(2, 'R^2') sage: c_cart.<x,y> = M.chart() # Cartesian coordinates on R^2 sage: E = M.vector_bundle(2, 'E') sage: e = E.local_frame('e') # makes E trivial sage: s = E.section(x+y, -1+x^2, name='s') sage: D = M.open_subset('D') # the unit open disc sage: e_D = e.restrict(D) sage: c_cart_D = c_cart.restrict(D, x^2+y^2<1) sage: s_D = s.restrict(D) ; s_D Section s on the Open subset D of the 2-dimensional differentiable manifold R^2 with values in the real vector bundle E of rank 2 ... sage: s_D[[1]] == s[[1]] False
With respect to the coercion model, the output is perfectly fine: the section s
can be restricted to D
. Thus there exists a coercion from sections on M
to sections on D
. The equality check then takes places on the sections on D
.
Should I simply rewrite this doctest, or what is the best procedure here?
For consistency, the same should then hold for chart functions, too. Meaning the following command right above should then return True
as well:
sage: s_D[1] == s[1]
comment:15 Changed 23 months ago by
- Commit changed from 03019c1601c3169932c0fff5a0abcfeaee2a3d71 to f265d5753f37e0a4cdd369f5e7471d7962726ccb
Branch pushed to git repo; I updated commit sha1. New commits:
f265d57 | Trac #30116: __ne__ method removed and trivial cases handled via checks
|
comment:16 Changed 23 months ago by
- Commit changed from f265d5753f37e0a4cdd369f5e7471d7962726ccb to 5f66a684abe885860337c69723b3499a5dbd6150
Branch pushed to git repo; I updated commit sha1. New commits:
5f66a68 | Trac #30116: __eq__ replaced by _richcmp_ for chart function
|
comment:17 Changed 23 months ago by
- Description modified (diff)
- Summary changed from Replace __eq__ by _richcmp_ for scalar and tensor fields to Replace __eq__ by _richcmp_ for manifolds
comment:18 Changed 23 months ago by
- Component changed from geometry to manifolds
comment:19 Changed 23 months ago by
- Cc mkoeppe added
comment:20 Changed 23 months ago by
- Description modified (diff)
comment:21 Changed 23 months ago by
- Description modified (diff)
comment:22 Changed 23 months ago by
- Description modified (diff)
comment:23 Changed 23 months ago by
- Description modified (diff)
comment:24 Changed 23 months ago by
- Description modified (diff)
comment:25 in reply to: ↑ 14 ; follow-up: ↓ 26 Changed 23 months ago by
Replying to gh-mjungmath:
With respect to the coercion model, the output is perfectly fine: the section
s
can be restricted toD
. Thus there exists a coercion from sections onM
to sections onD
. The equality check then takes places on the sections onD
.Should I simply rewrite this doctest, or what is the best procedure here?
For consistency, the same should then hold for chart functions, too. Meaning the following command right above should then return
True
as well:sage: s_D[1] == s[1]
I am a little bit worried about this: s_D[1]
and s[1]
are mathematically distinct objects (they don't have the same domain), so the comparison output should be False
, despite s[1]
can be coerced (by restriction) to a function with the same domain as s_D[1]
and the coercion outcome is equal to s_D[1]
.
comment:26 in reply to: ↑ 25 ; follow-up: ↓ 29 Changed 23 months ago by
Replying to egourgoulhon:
I am a little bit worried about this:
s_D[1]
ands[1]
are mathematically distinct objects (they don't have the same domain), so the comparison output should beFalse
, despites[1]
can be coerced (by restriction) to a function with the same domain ass_D[1]
and the coercion outcome is equal tos_D[1]
.
For matrices, things are even more distracting than in our particular case:
sage: MatrixSpace(QQ, 3, 3)(2) [2 0 0] [0 2 0] [0 0 2] sage: MatrixSpace(QQ, 3, 3)(2) == 2 True
Digging in the documentation, I've found:
The primary goal of coercion is to be able to transparently do arithmetic, comparisons, etc. between elements of distinct sets.
From this perspective, the matrix code example above makes somewhat sense, and so it would in our case.
comment:27 Changed 23 months ago by
- Commit changed from 5f66a684abe885860337c69723b3499a5dbd6150 to 11c9d35deade99b7a8cb186b4edfa507c74ff213
comment:28 Changed 23 months ago by
- Description modified (diff)
comment:29 in reply to: ↑ 26 ; follow-up: ↓ 30 Changed 22 months ago by
Replying to gh-mjungmath:
Replying to egourgoulhon:
I am a little bit worried about this:
s_D[1]
ands[1]
are mathematically distinct objects (they don't have the same domain), so the comparison output should beFalse
, despites[1]
can be coerced (by restriction) to a function with the same domain ass_D[1]
and the coercion outcome is equal tos_D[1]
.For matrices, things are even more distracting than in our particular case:
sage: MatrixSpace(QQ, 3, 3)(2) [2 0 0] [0 2 0] [0 0 2] sage: MatrixSpace(QQ, 3, 3)(2) == 2 TrueDigging in the documentation, I've found:
The primary goal of coercion is to be able to transparently do arithmetic, comparisons, etc. between elements of distinct sets.
From this perspective, the matrix code example above makes somewhat sense, and so it would in our case.
Allow me to give another example:
sage: GF(5)(7) == 2 True
We cannot recover 7
from the finite field again. Furthermore the representative of 7
in GF(5)
and the number 2
are certainly two distinct mathematical objects. Sure, this equality must be seen in terms of congruences. Now, I think we can see the equality of restricted functions somehow similarly. (Forget about the sheafs, that was nonsense.)
comment:30 in reply to: ↑ 29 ; follow-up: ↓ 31 Changed 22 months ago by
Replying to gh-mjungmath:
For matrices, things are even more distracting than in our particular case:
sage: MatrixSpace(QQ, 3, 3)(2) == 2 TrueAllow me to give another example:
sage: GF(5)(7) == 2 True
Thanks for providing these examples. I feel slightly more comfortable with comparison with the integers, since it's obvious that we are dealing with distinct mathematical entities and the equality results from some canonical identification. For tensor fields with the coercions based on restrictions, we are dealing with the same type of entities, so it seems to me that having a == b
yielding True
when a
and b
are two tensor fields defined on distinct domains is more dangerous than having e.g. a == 0
return True
. I would be curious to have the opinion of Travis and Matthias on that...
comment:31 in reply to: ↑ 30 ; follow-up: ↓ 32 Changed 22 months ago by
Replying to egourgoulhon:
Thanks for providing these examples. I feel slightly more comfortable with comparison with the integers, since it's obvious that we are dealing with distinct mathematical entities and the equality results from some canonical identification. For tensor fields with the coercions based on restrictions, we are dealing with the same type of entities, so it seems to me that having
a == b
yieldingTrue
whena
andb
are two tensor fields defined on distinct domains is more dangerous than having e.g.a == 0
returnTrue
. I would be curious to have the opinion of Travis and Matthias on that...
Mh, okay. I see your point. Nevertheles, we allow addition and multiplication between a scalar field and a restricted scalar field, which comes with the very same problem, doesn't it?
Imho, there should be no coercion for restrictions when we do not allow a comparison for them. Simply because they don't fit into the coercion model then.
I am curious about their opinion, too. :)
comment:32 in reply to: ↑ 31 ; follow-up: ↓ 34 Changed 22 months ago by
Replying to gh-mjungmath:
Replying to egourgoulhon:
Thanks for providing these examples. I feel slightly more comfortable with comparison with the integers, since it's obvious that we are dealing with distinct mathematical entities and the equality results from some canonical identification. For tensor fields with the coercions based on restrictions, we are dealing with the same type of entities, so it seems to me that having
a == b
yieldingTrue
whena
andb
are two tensor fields defined on distinct domains is more dangerous than having e.g.a == 0
returnTrue
. I would be curious to have the opinion of Travis and Matthias on that...Mh, okay. I see your point. Nevertheles, we allow addition and multiplication between a scalar field and a restricted scalar field,
indeed.
which comes with the very same problem, doesn't it?
Maybe it is a matter of taste but a*b
resulting in a scalar field on the common subdomain of a
and b
seems less error prone to me than a == b
returning True
.
Imho, there should be no coercion for restrictions when we do not allow a comparison for them. Simply because they don't fit into the coercion model then.
I agree that to fully fit with the coercion model, we should allow comparison as proposed in this ticket.
However, one can still allow some kind of conversions right?
I am curious about their opinion, too. :)
In particular, are there other structures in Sage that implement the coercion model but for comparison?
comment:33 follow-up: ↓ 36 Changed 22 months ago by
Okay, I thought about it once again, and I tend to agree. If we would apply the changes I propose, the following code would return True
:
sage: M = Manifold(2, 'M') sage: U = M.open_subset('U') sage: f = M.scalar_field('f') sage: A_U = U.scalar_field_algebra() sage: f in A_U True
And as you, Eric, already pointed out, that is definitely not what we want.
If I understand this correctly, and two coercible elements must always be comparable, I vote for keeping the conversion:
sage: A_U(f) in A_U True
but removing the coercion from the populated list, i.e.
- elif isinstance(other, ScalarFieldAlgebra):
- return self._domain.is_subset(other._domain)
in _coerce_map_from_
of the file scalarfield_algebra.py
.
I know, this is inconvenient when it comes to addition and multiplication of restricted elements. On the other hand, it would be consistent with Sage's coercion model then. It is therefore mandatory to investigate if coercible elements must always be comparable or not.
Addendum: see https://doc.sagemath.org/html/en/thematic_tutorials/coercion_and_categories.html#equality-and-element-containment for details.
As pointed out in this article, one could also simply overwrite __contains__
and still keep the equality test...
comment:34 in reply to: ↑ 32 ; follow-up: ↓ 35 Changed 22 months ago by
Replying to egourgoulhon:
Maybe it is a matter of taste but
a*b
resulting in a scalar field on the common subdomain ofa
andb
seems less error prone to me thana == b
returningTrue
.
If you want the multiplication (by coercion), then you have to also allow the equality check unless you explicitly disallow comparisons under coercion.
The equality test in both ways makes sense, it just depends on whether you want to consider them as completely different functions (with incompatible domains) or you want to do an actual comparison of the functions on the same common domain. The issue is ==
inherently does not ask you to specify the type of equality, which we generally assume from context (usually by adding prose when saying two functions are equal). We leave the choice of the semantics of ==
up to the developer of a particular block of code. Yet, the existence of the coercion means the two objects should inherently be (naturally) relatable.
If you still want the multiplication and do not want the equality, then you can implement the multiplication as an action (with the M
scalar field acting on the U
one).
My opinion is to use the _richcmp_
and just make a note that when comparing maps (e.g., a scalar field) in a manifold, it restricts to the common domain and does the comparison there. If a user wants to explicit check that the maps have different domains, then they will have to add an extra check of the domain()
s.
TL;DR It depends on what you think is the most natural semantic, and it is your choice.
comment:35 in reply to: ↑ 34 ; follow-up: ↓ 37 Changed 22 months ago by
Replying to tscrim:
My opinion is to use the
_richcmp_
and just make a note that when comparing maps (e.g., a scalar field) in a manifold, it restricts to the common domain and does the comparison there. If a user wants to explicit check that the maps have different domains, then they will have to add an extra check of thedomain()
s.TL;DR It depends on what you think is the most natural semantic, and it is your choice.
Thanks for your explanations and advice.
Since the coercion of tensor fields is very handy, not only for multiplication (in which case we could use an action as you pointed out) by also for addition/subtraction and multilinear form operations like g(u,v)
with u
and v
vector fields defined on subsets of the domain of metric g
, I would suggest to keep the coercion and use _richcmp_
as proposed in this ticket, with some doctests warning about the behaviour of the equality operator.
comment:36 in reply to: ↑ 33 Changed 22 months ago by
Replying to gh-mjungmath:
As pointed out in this article, one could also simply overwrite
__contains__
and still keep the equality test...
Indeed!
comment:37 in reply to: ↑ 35 ; follow-up: ↓ 38 Changed 22 months ago by
Replying to egourgoulhon:
Replying to tscrim:
My opinion is to use the
_richcmp_
and just make a note that when comparing maps (e.g., a scalar field) in a manifold, it restricts to the common domain and does the comparison there. If a user wants to explicit check that the maps have different domains, then they will have to add an extra check of thedomain()
s.TL;DR It depends on what you think is the most natural semantic, and it is your choice.
Thanks for your explanations and advice. Since the coercion of tensor fields is very handy, not only for multiplication (in which case we could use an action as you pointed out) by also for addition/subtraction and multilinear form operations like
g(u,v)
withu
andv
vector fields defined on subsets of the domain of metricg
, I would suggest to keep the coercion and use_richcmp_
as proposed in this ticket, with some doctests warning about the behaviour of the equality operator.
In that case, I would overwrite __contains__
and add this exception since M.scalar_field() in U.scalar_field_algebra()
returning True
is certainly unwanted. Agreed? Should I proceed similarly for the coercion implemented in #30112? This coercion is also convenient, but the containment check should return False
, too.
comment:38 in reply to: ↑ 37 Changed 22 months ago by
Replying to gh-mjungmath:
Replying to egourgoulhon:
Thanks for your explanations and advice. Since the coercion of tensor fields is very handy, not only for multiplication (in which case we could use an action as you pointed out) by also for addition/subtraction and multilinear form operations like
g(u,v)
withu
andv
vector fields defined on subsets of the domain of metricg
, I would suggest to keep the coercion and use_richcmp_
as proposed in this ticket, with some doctests warning about the behaviour of the equality operator.In that case, I would overwrite
__contains__
and add this exception sinceM.scalar_field() in U.scalar_field_algebra()
returningTrue
is certainly unwanted. Agreed?
Yes.
Should I proceed similarly for the coercion implemented in #30112? This coercion is also convenient, but the containment check should return
False
, too.
Indeed.
comment:39 Changed 22 months ago by
While trying to write a __contains__
method, I have encountered other things I am worried about. Namely, in the current state, we have:
sage: M = Manifold(2, 'M') sage: U = M.open_subset('U') sage: X.<x,y> = U.chart() sage: x in M.scalar_field_algebra() True sage: X.function_ring()(x) in M.scalar_field_algebra() True
The latter containment check will return False
after this change. The former, however, will still return True
. This comes from the coercion:
sage: A = M.scalar_field_algebra() sage: A.has_coerce_map_from(SR) True
This is not unproblematic: the symbolic variable x
will be coerced into the scalar field which is defined as x
on U
. But this is no coercion via restriction and therefore, due to our previous discussion, not even a coercion at all!
Suggestions? Opinions?
comment:40 follow-up: ↓ 41 Changed 22 months ago by
It seems like you are conflating two things: the generic-ness of the SR variable x
, which is considered here as an element of the base ring of the scalar_field_algebra()
(which is not a map, a priori, but is identified with elements/maps in the algebra) and the map on the function ring of the chart X
of U
. Since you want containment to include the domain (which I think is natural), then the latter should return false. So I don't see the problem.
comment:41 in reply to: ↑ 40 Changed 22 months ago by
Replying to tscrim:
It seems like you are conflating two things: the generic-ness of the SR variable
x
, which is considered here as an element of the base ring of thescalar_field_algebra()
(which is not a map, a priori, but is identified with elements/maps in the algebra) and the map on the function ring of the chartX
ofU
. Since you want containment to include the domain (which I think is natural), then the latter should return false. So I don't see the problem.
I think that is not entirely correct. The symbolic variable x
cannot be considered as an element of the scalar field algebra on M
, at least not uniquely. Namely, it is not (well-)defined on M \ U
. Constants are fine. But as soon as a symbolic variable corresponds to a chart label, this setting gets messed up. For example:
sage: M = Manifold(2, 'M') sage: U = M.open_subset('U'); V = M.open_subset('V') sage: XU.<x,y> = U.chart(); XV.<z,t> = V.chart() sage: M.declare_union(U, V) sage: f = M.scalar_field_algebra()(x+z) sage: f.display() M --> R sage: f._expres {}
In the last point of my post, I was more referring to the coercion itself. Let us take a similar example to the one above:
sage: M = Manifold(2, 'M') sage: U = M.open_subset('U'); V = M.open_subset('V') sage: XU.<x,y> = U.chart(); XV.<z,t> = V.chart() sage: M.declare_union(U, V) sage: f = U.scalar_field_algebra()(z) sage: f.display() U --> R on V: (z, t) |--> z sage: g = M.scalar_field_algebra()(z) sage: g.display() M --> R on V: (z, t) |--> z sage: g_U = U.scalar_field_algebra()(g) sage: g_U.display() U --> R
Coercions have to follow strict axioms (see https://doc.sagemath.org/html/en/thematic_tutorials/coercion_and_categories.html#the-four-axioms-requested-for-coercions). This is a violation of axiom 3, i.e. g_U
must yield f
. That the base ring is SR
makes it even worse in this case.
This issue is rather critical. However, I have no idea how we can solve this except for changing the entire base ring or removing the coercion given by restrictions.
comment:42 follow-up: ↓ 43 Changed 22 months ago by
Regarding comment:24 at #30191, we even have that GF(5).zero() in ZZ
yields True
. Is that an evidence that we should keep M.scalar_field() in U.scalar_field_algebra()
as being True
?
I think, the issue noticed in comment:41 should be discussed in another ticket so that we can close this ticket soon.
comment:43 in reply to: ↑ 42 Changed 22 months ago by
Replying to gh-mjungmath:
I think, the issue noticed in comment:41 should be discussed in another ticket so that we can close this ticket soon.
Indeed.
comment:44 Changed 22 months ago by
I would suggest we keep the M.scalar_field() in U.scalar_field_algebra()
yielding True
behavior for now and discuss that in a subsequent ticket. Do you agree?
comment:45 Changed 22 months ago by
- Dependencies changed from #30112 to #30112, #30267
- Description modified (diff)
comment:46 Changed 22 months ago by
- Commit changed from 11c9d35deade99b7a8cb186b4edfa507c74ff213 to a59c71acdf7d22fcafd00ad1965407cf9f9b8a12
Branch pushed to git repo; I updated commit sha1. New commits:
a59c71a | Trac #30267: coercion via restriction added
|
comment:47 Changed 22 months ago by
- Commit changed from a59c71acdf7d22fcafd00ad1965407cf9f9b8a12 to a9d652faa628d3f7cc9f747ef83f0000ba53f2d5
Branch pushed to git repo; I updated commit sha1. New commits:
3e2b28c | Trac #30267: coercion via restriction added
|
f640f21 | Trac #30116: Merge branch 't/30267/coercion_via_restriction_of_chart_functions' into t/30116/test_comparison_tensorfield
|
4bcd2ee | Trac #30267: coercion via restriction added
|
89906e2 | Trac #30116: Merge branch 'coercion_via_restriction_of_chart_functions' into t/30116/test_comparison_tensorfield
|
a9d652f | Trac #30116: documentation adapted
|
comment:48 Changed 22 months ago by
- Status changed from needs_info to needs_review
comment:49 Changed 22 months ago by
Yes, I think we can push that to another ticket since I would consider it a different issue.
comment:50 follow-up: ↓ 51 Changed 22 months ago by
Okay, then this ticket is ready for review. Patchbot is green btw.
comment:51 in reply to: ↑ 50 Changed 22 months ago by
comment:52 follow-up: ↓ 53 Changed 22 months ago by
- Reviewers set to Eric Gourgoulhon, Travis Scrimshaw
- Status changed from needs_review to positive_review
LGTM. Thanks! Travis, do you agree?
FWIW, I've run some benchmarks for some of the notebooks of the examples page. Here are the results in seconds on a Core i7-6700HQ + 16 GB RAM computer:
len(M.atlas()) Sage 9.2.beta6 this ticket SM_sphere_S2 7 231 s 224 s SM_sphere_S3_Hopf 18 379 s 380 s SM_anti_de_Sitter 10 416 s 421 s SM_Schwarzschild 11 253 s 253 s SM_Kerr 1 494 s 485 s
So we don't observe any significant performance regression; it's even slightly better for some notebooks.
comment:53 in reply to: ↑ 52 Changed 22 months ago by
Replying to egourgoulhon:
LGTM. Thanks! Travis, do you agree?
FWIW, I've run some benchmarks for some of the notebooks of the examples page. Here are the results in seconds on a Core i7-6700HQ + 16 GB RAM computer:
len(M.atlas()) Sage 9.2.beta6 this ticket SM_sphere_S2 7 231 s 224 s SM_sphere_S3_Hopf 18 379 s 380 s SM_anti_de_Sitter 10 416 s 421 s SM_Schwarzschild 11 253 s 253 s SM_Kerr 1 494 s 485 sSo we don't observe any significant performance regression; it's even slightly better for some notebooks.
Thank you for the positive review and the benchmarks. What would probably be more interesting is a benchmarking of #30191, I guess.
comment:54 Changed 22 months ago by
- Commit changed from a9d652faa628d3f7cc9f747ef83f0000ba53f2d5 to 2c06105b9ea437e97c35a936de5a7cebabdd6003
- Status changed from positive_review to needs_review
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
2c06105 | Trac #30116: Merge branch 'develop' into t/30116/test_comparison_tensorfield
|
comment:55 Changed 22 months ago by
There was a merge conflict with the new beta. The cause was #30108. Can I switch back to positive review when the bot turns green?
comment:56 Changed 22 months ago by
- Status changed from needs_review to positive_review
Morally green patchbot.
comment:52 response: I have no objections.
comment:57 follow-up: ↓ 59 Changed 22 months ago by
- Status changed from positive_review to needs_info
Matthias mentioned a very important point. The equality via restriction doesn't satisfy the transitivity condition. That is because the scalar fields are not necessarily analytic. That's really bad.
Is there a compromise, namely keeping _richcmp_
and the coercion but separate this particular case? I don't know of any.
comment:58 Changed 22 months ago by
As I mentioned in #30266, I don't think you should feel obligated to use the coercion framework for comparisons just because you use it for arithmetic.
I would suggest to compile a set of examples that really illustrate what elements you want to compare equal, to make sure that this is an equivalence relation, and then to figure out if coercion is the right tool to implement this.
Perhaps the question should be asked in a broader context of coercion within the category of partial maps.
comment:59 in reply to: ↑ 57 Changed 22 months ago by
Replying to gh-mjungmath:
Matthias mentioned a very important point. The equality via restriction doesn't satisfy the transitivity condition. That is because the scalar fields are not necessarily analytic. That's really bad.
That sounds like an issue with the coercions. However, if you want them to also compare as equals without the coercion, then you will need to simply implement an __eq__
that separately checks this as a fallback provided the first attempt via the coercion framework doesn't work.
comment:60 Changed 19 months ago by
- Milestone changed from sage-9.2 to sage-9.3
comment:61 Changed 17 months ago by
Okay, just to summarize for me: what is the punchline of our discussion and where should this ticket go?
comment:62 Changed 16 months ago by
I would say Matthias's comment:58 is what should be done: provide some examples of things you want and do not want to inform you of the policy you want to set.
comment:63 Changed 14 months ago by
- Milestone changed from sage-9.3 to sage-9.4
Setting new milestone based on a cursory review of ticket status, priority, and last modification date.
comment:64 Changed 13 months ago by
Possibly related: #31703
comment:65 Changed 10 months ago by
- Milestone changed from sage-9.4 to sage-9.5
Setting a new milestone for this ticket based on a cursory review.
comment:66 Changed 5 months ago by
- Milestone changed from sage-9.5 to sage-9.6
Stalled in needs_review
or needs_info
; likely won't make it into Sage 9.5.
comment:67 Changed 3 months ago by
- Milestone changed from sage-9.6 to sage-9.7
New commits:
Trac #30108: != bug fixed
Trac #30108: eq check between scalar fields and mixed forms
Trac #30108: wrong import
__eq__ replaced by _richcmp_
Trac #30108: unused import richcmp removed
Merge branch 'mixed_form_inequality_bug' into test_comparison_tensorfield
Trac #30112: coerce map from chart function ring added
Merge branch 't/30112/coercion_from_chartfunction_to_scalarfield' into test_comparison_tensorfield