Replace __eq__ by _richcmp_ for manifolds

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

[gh-mjungmath]

New commits:

​3eba4e1	Trac #30108: != bug fixed
​aa605ab	Trac #30108: eq check between scalar fields and mixed forms
​b7d9f47	Trac #30108: wrong import
​d798b17	__eq__ replaced by _richcmp_
​0b93b22	Trac #30108: unused import richcmp removed
​2c4e13c	Merge branch 'mixed_form_inequality_bug' into test_comparison_tensorfield
​8b98301	Trac #30112: coerce map from chart function ring added
​6c28c72	Merge branch 't/30112/coercion_from_chartfunction_to_scalarfield' into test_comparison_tensorfield

[git]

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

​979c815

Trac #30116: __eq__ replaced by _richcmp_

[git]

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

​5fcb2e7

Trac #30116: input info op added

[git]

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

​0089c6e	Trac #30112: coercion for differentiable scalar fields
​00e647d	Trac #30116: Merge branch 't/30112/coercion_from_chartfunction_to_scalarfield' into replace_eq_by_richcmp_tensorfield

[git]

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

​385c4f9	Trac #30112: doctest added
​86a5438	Trac #30116: Merge branch 't/30112/coercion_from_chartfunction_to_scalarfield' into replace_eq_by_richcmp_tensorfield

[git]

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

​03019c1

Trac #30116: unused import removed + replacement for sections

[gh-mjungmath]

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]

[git]

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

​f265d57

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

[git]

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

​5f66a68

Trac #30116: __eq__ replaced by _richcmp_ for chart function

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

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

[git]

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

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

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

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

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

[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?

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

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

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

[egourgoulhon]

Replying to gh-mjungmath:

As pointed out in this article, one could also simply overwrite __contains__ and still keep the equality test...

Indeed!

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

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

[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?

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

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

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

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

[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?

[git]

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

​a59c71a

Trac #30267: coercion via restriction added

[git]

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

[tscrim]

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

[gh-mjungmath]

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

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

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

[git]

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

[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?

[tscrim]

Morally green patchbot.

comment:52 response: I have no objections.

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

Is there a compromise, namely keeping _richcmp_ and the coercion but separate this particular case? I don't know of any.

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

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

[gh-mjungmath]

Okay, just to summarize for me: what is the punchline of our discussion and where should this ticket go?

[tscrim]

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.

[mkoeppe]

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.

[gh-mjungmath]

Possibly related: #31703

[mkoeppe]

Setting a new milestone for this ticket based on a cursory review.

[mkoeppe]

Stalled in needs_review or needs_info; likely won't make it into Sage 9.5.