Opened 5 months ago

Last modified 6 weeks ago

#30116 needs_info defect

Replace __eq__ by _richcmp_ for manifolds

Reported by: gh-mjungmath Owned by:
Priority: major Milestone: sage-9.3
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) Commit: 2c06105b9ea437e97c35a936de5a7cebabdd6003
Dependencies: #30112, #30267 Stopgaps:

Description (last modified by gh-mjungmath)

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 (60)

comment:1 Changed 5 months ago by gh-mjungmath

  • Dependencies set to #30112

comment:2 Changed 5 months ago by gh-mjungmath

  • Branch set to u/gh-mjungmath/test_comparison_tensorfield

comment:3 Changed 5 months ago by gh-mjungmath

  • Commit set to 6c28c72b3ffb2ceb34b76f007b47b84713d4d0bb
  • Dependencies changed from #30112 to #30112, #30108

New commits:

3eba4e1Trac #30108: != bug fixed
aa605abTrac #30108: eq check between scalar fields and mixed forms
b7d9f47Trac #30108: wrong import
d798b17__eq__ replaced by _richcmp_
0b93b22Trac #30108: unused import richcmp removed
2c4e13cMerge branch 'mixed_form_inequality_bug' into test_comparison_tensorfield
8b98301Trac #30112: coerce map from chart function ring added
6c28c72Merge branch 't/30112/coercion_from_chartfunction_to_scalarfield' into test_comparison_tensorfield

comment:4 Changed 5 months ago by git

  • Commit changed from 6c28c72b3ffb2ceb34b76f007b47b84713d4d0bb to 979c815a40b1c96ab8ed2e806afd95e8d5078056

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

979c815Trac #30116: __eq__ replaced by _richcmp_

comment:5 Changed 5 months ago by gh-mjungmath

  • Dependencies changed from #30112, #30108 to #30112

comment:6 Changed 5 months ago by git

  • Commit changed from 979c815a40b1c96ab8ed2e806afd95e8d5078056 to 5fcb2e7725cf05fd7243411f1fa98a6aabee578a

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

5fcb2e7Trac #30116: input info op added

comment:7 Changed 5 months ago by gh-mjungmath

  • Status changed from new to needs_review

comment:8 Changed 5 months ago by gh-mjungmath

  • Status changed from needs_review to needs_work

comment:9 Changed 5 months ago by git

  • Commit changed from 5fcb2e7725cf05fd7243411f1fa98a6aabee578a to 00e647d8c7c877bd1acb53e612ef0ecfda3150ad

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

0089c6eTrac #30112: coercion for differentiable scalar fields
00e647dTrac #30116: Merge branch 't/30112/coercion_from_chartfunction_to_scalarfield' into replace_eq_by_richcmp_tensorfield

comment:10 Changed 5 months ago by git

  • Commit changed from 00e647d8c7c877bd1acb53e612ef0ecfda3150ad to 86a5438f8f82f718e96b60f3a6f4886bdd176114

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

385c4f9Trac #30112: doctest added
86a5438Trac #30116: Merge branch 't/30112/coercion_from_chartfunction_to_scalarfield' into replace_eq_by_richcmp_tensorfield

comment:11 Changed 5 months ago by gh-mjungmath

  • Status changed from needs_work to needs_info

comment:12 Changed 5 months ago by git

  • Commit changed from 86a5438f8f82f718e96b60f3a6f4886bdd176114 to 03019c1601c3169932c0fff5a0abcfeaee2a3d71

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

03019c1Trac #30116: unused import removed + replacement for sections

comment:13 Changed 5 months ago by gh-mjungmath

  • Status changed from needs_info to needs_review

comment:14 follow-up: Changed 5 months ago by gh-mjungmath

  • 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 5 months ago by git

  • Commit changed from 03019c1601c3169932c0fff5a0abcfeaee2a3d71 to f265d5753f37e0a4cdd369f5e7471d7962726ccb

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

f265d57Trac #30116: __ne__ method removed and trivial cases handled via checks

comment:16 Changed 5 months ago by git

  • Commit changed from f265d5753f37e0a4cdd369f5e7471d7962726ccb to 5f66a684abe885860337c69723b3499a5dbd6150

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

5f66a68Trac #30116: __eq__ replaced by _richcmp_ for chart function

comment:17 Changed 5 months ago by gh-mjungmath

  • 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 5 months ago by gh-mjungmath

  • Component changed from geometry to manifolds

comment:19 Changed 5 months ago by gh-mjungmath

  • Cc mkoeppe added

comment:20 Changed 5 months ago by gh-mjungmath

  • Description modified (diff)

comment:21 Changed 5 months ago by gh-mjungmath

  • Description modified (diff)

comment:22 Changed 5 months ago by gh-mjungmath

  • Description modified (diff)

comment:23 Changed 5 months ago by gh-mjungmath

  • Description modified (diff)

comment:24 Changed 5 months ago by gh-mjungmath

  • Description modified (diff)

comment:25 in reply to: ↑ 14 ; follow-up: Changed 5 months ago by egourgoulhon

Replying to gh-mjungmath:

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]

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: Changed 5 months ago by gh-mjungmath

Replying to egourgoulhon:

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

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 5 months ago by git

  • Commit changed from 5f66a684abe885860337c69723b3499a5dbd6150 to 11c9d35deade99b7a8cb186b4edfa507c74ff213

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

22e2961Trac #30116: Merge branch 'develop' into replace_eq_by_richcmp_tensorfield
11c9d35Trac #30116: invoke super method by using super() + replacing __eq__ in most files

comment:28 Changed 5 months ago by gh-mjungmath

  • Description modified (diff)

comment:29 in reply to: ↑ 26 ; follow-up: Changed 5 months ago by gh-mjungmath

Replying to gh-mjungmath:

Replying to egourgoulhon:

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

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.

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

Last edited 5 months ago by gh-mjungmath (previous) (diff)

comment:30 in reply to: ↑ 29 ; follow-up: Changed 5 months ago by egourgoulhon

Replying to gh-mjungmath:

For matrices, things are even more distracting than in our particular case:

sage: MatrixSpace(QQ, 3, 3)(2) == 2
True 

Allow 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: Changed 5 months ago by 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 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...

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

Last edited 5 months ago by gh-mjungmath (previous) (diff)

comment:32 in reply to: ↑ 31 ; follow-up: Changed 5 months ago by egourgoulhon

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

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: Changed 5 months ago by gh-mjungmath

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

Last edited 5 months ago by gh-mjungmath (previous) (diff)

comment:34 in reply to: ↑ 32 ; follow-up: Changed 5 months ago by tscrim

Replying to egourgoulhon:

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.

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: Changed 5 months ago by 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 the domain()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 5 months ago by egourgoulhon

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: Changed 5 months ago by gh-mjungmath

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 the domain()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.

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 5 months ago by egourgoulhon

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

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?

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 5 months ago by gh-mjungmath

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: Changed 5 months ago by 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 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 5 months ago by gh-mjungmath

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

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.

Last edited 5 months ago by gh-mjungmath (previous) (diff)

comment:42 follow-up: Changed 4 months ago by gh-mjungmath

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 4 months ago by egourgoulhon

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.

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

comment:44 Changed 4 months ago by gh-mjungmath

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 4 months ago by gh-mjungmath

  • Dependencies changed from #30112 to #30112, #30267
  • Description modified (diff)

comment:46 Changed 4 months ago by git

  • Commit changed from 11c9d35deade99b7a8cb186b4edfa507c74ff213 to a59c71acdf7d22fcafd00ad1965407cf9f9b8a12

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

a59c71aTrac #30267: coercion via restriction added

comment:47 Changed 4 months ago by git

  • Commit changed from a59c71acdf7d22fcafd00ad1965407cf9f9b8a12 to a9d652faa628d3f7cc9f747ef83f0000ba53f2d5

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

3e2b28cTrac #30267: coercion via restriction added
f640f21Trac #30116: Merge branch 't/30267/coercion_via_restriction_of_chart_functions' into t/30116/test_comparison_tensorfield
4bcd2eeTrac #30267: coercion via restriction added
89906e2Trac #30116: Merge branch 'coercion_via_restriction_of_chart_functions' into t/30116/test_comparison_tensorfield
a9d652fTrac #30116: documentation adapted

comment:48 Changed 4 months ago by gh-mjungmath

  • Status changed from needs_info to needs_review

comment:49 Changed 4 months ago by tscrim

Yes, I think we can push that to another ticket since I would consider it a different issue.

comment:50 follow-up: Changed 4 months ago by gh-mjungmath

Okay, then this ticket is ready for review. Patchbot is green btw.

Last edited 4 months ago by gh-mjungmath (previous) (diff)

comment:51 in reply to: ↑ 50 Changed 4 months ago by gh-mjungmath

Last edited 4 months ago by gh-mjungmath (previous) (diff)

comment:52 follow-up: Changed 4 months ago by egourgoulhon

  • 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 4 months ago by gh-mjungmath

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 s

So 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 4 months ago by git

  • 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:

2c06105Trac #30116: Merge branch 'develop' into t/30116/test_comparison_tensorfield

comment:55 Changed 4 months ago by gh-mjungmath

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 4 months ago by tscrim

  • Status changed from needs_review to positive_review

Morally green patchbot.

comment:52 response: I have no objections.

comment:57 follow-up: Changed 4 months ago by gh-mjungmath

  • 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 4 months ago by mkoeppe

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 4 months ago by tscrim

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 6 weeks ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3
Note: See TracTickets for help on using tickets.