Immutability for scalar fields

[Michael Jung]

This ticket adds the immutability and hashability for scalar fields.

See #30261.

[Michael Jung]

New commits:

​286d3c6	FreeModuleTensor, Vector: Subclass a new class ModuleElementWithMutability
​9df3d22	sage.tensor.modules: Make all zero() and one() elements immutable
​4373ea2	FreeModuleTensor: Replace special casing of the immutable zero by is_immutable
​a2ee8be	ModuleElementWithMutability.is_immutable, is_mutable: Change def to cpdef
​aebaded	Trac #30266: make scalarfields immutable

[Michael Jung]

Is the hash input okay? Or should it be something more unique?

[Matthias Köppe]

Given that ScalarField_repr_ does not print any contents, this hash does not seem to be useful to distinguish non-equal elements. This will degrade the performance of sets to that of lists.

More importantly, it must be ensured that f == g implies hash(f) == hash(g). Because equality does not take names into account but repr does, this is currently not satisfied.

[Michael Jung]

Replying to mkoeppe:

Given that ScalarField_repr_ does not print any contents, this hash does not seem to be useful to distinguish non-equal elements. This will degrade the performance of sets to that of lists.

More importantly, it must be ensured that f == g implies hash(f) == hash(g). Because equality does not take names into account but repr does, this is currently not satisfied.

I just copied the ideas from the elsewhere in the Sage code. This means that most hash functions are implemented wrong.

Are you sure it is not f is g implies hash(f) == hash(g)? f == g implies hash(f) == hash(g) is barely possible in cases where coercions apply, isn't it?

[Matthias Köppe]

Replying to gh-mjungmath:

Are you sure it is not f is g implies hash(f) == hash(g)?

This implication is trivially true.

[Michael Jung]

Replying to mkoeppe:

Replying to gh-mjungmath:

Are you sure it is not f is g implies hash(f) == hash(g)?

This implication is trivially true.

Oh, yes, this is obviously right. >.< :D

[Michael Jung]

Replying to gh-mjungmath:

Replying to mkoeppe:

Given that ScalarField_repr_ does not print any contents, this hash does not seem to be useful to distinguish non-equal elements. This will degrade the performance of sets to that of lists.

More importantly, it must be ensured that f == g implies hash(f) == hash(g). Because equality does not take names into account but repr does, this is currently not satisfied.

I just copied the ideas from the elsewhere in the Sage code. This means that most hash functions are implemented wrong.

Ah yes, I see, apparently that were cases where the string representation cannot be changed. Due to the fact that we allow restrictions as coercion, hash(self._manifold) seem to be the only choice.

I will add a related doctest for this particular case.

Thank you.

[git]

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

​5c7fd6e

Trac #30266: immutability of restrictions + hash function improved

[Michael Jung]

This should be better.

[Michael Jung]

Is that hash function okay? I have added a short f == g implies hash(f) == hash(g) test.

Patchbot is green, btw.

[Michael Jung]

Since #30116, scalar fields can be compared on common domains. I added the check of the hash condition in the doctest for this particular comparison.

[git]

Branch pushed to git repo; I updated commit sha1. Last 10 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
​a59c71a	Trac #30267: coercion via restriction added
​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
​055d226	Trac #30266: Merge branch 't/30116/test_comparison_tensorfield' into t/30266/immutability_for_scalar_fields
​8ca8b75	Trac #30266: hash compatibility check wrt #30116

[git]

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

​0bf9f36

Trac #30266: Merge branch 'develop' into t/30266/immutability_for_scalar_fields

[Michael Jung]

Patchbot is green.

[Travis Scrimshaw]

I believe for _richcmp_ you also have self is not other, but you should double check this.

[Michael Jung]

Replying to tscrim:

I believe for _richcmp_ you also have self is not other, but you should double check this.

In #30116, I used if self is other: instead. Is that what you mean?

[Travis Scrimshaw]

Replying to gh-mjungmath:

Replying to tscrim:

I believe for _richcmp_ you also have self is not other, but you should double check this.

In #30116, I used if self is other: instead. Is that what you mean?

Ah, I missed it there. Yes, I mean that, and I think it is unnecessary because the coercion framework through __eq__ handles that case. However, I don't trust myself here, so it is worth checking.

[Michael Jung]

I changed the whole _richcmp_ to return False and tried f == f. It returned False. So, no prior instance check here. For fun, I did the same with the opposite: no prior check there, too.

[git]

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

​a5f6a19

Trac 30266: hash function slightly improved

[Travis Scrimshaw]

Replying to gh-mjungmath:

I changed the whole _richcmp_ to return False and tried f == f. It returned False. So, no prior instance check here. For fun, I did the same with the opposite: no prior check there, too.

Thank you for checking. :)

[Travis Scrimshaw]

Unfortunately I am too busy right now to do the review. I can probably do it next week, but if someone else wants to step up and do it, please go ahead.

[Michael Jung]

One question btw: must 1 == f also imply hash(1) == hash(f). The former can be fulfilled due to the coercion framework, the latter is rather difficult since both have different types. Is it enough for this condition to be fulfilled for objects of the same type?

[Michael Jung]

For the ring of integers modulo some number, this behavior is not satisfied:

sage: 3 == GF(2)(3)
True
sage: hash(3) == hash(GF(2)(3))
False

[Matthias Köppe]

Ensuring that this implication holds across coercion is very difficult, and it is done in Sage on a "best effort" basis -- such as for QQ as the fraction field of ZZ.

The rule of thumb is that if we form sets, they should be of elements of the same parent so that no coercion is involved. Likewise for the keys of dictionaries.

[Michael Jung]

Replying to mkoeppe:

Ensuring that this implication holds across coercion is very difficult, and it is done in Sage on a "best effort" basis -- such as for QQ as the fraction field of ZZ.

The rule of thumb is that if we form sets, they should be of elements of the same parent so that no coercion is involved. Likewise for the keys of dictionaries.

Since the restriction is set as a coercion, and they live in different sets, maybe we should make the hash more unique? Namely:

-        return hash((type(self).__name__, self._manifold))
+        return hash((type(self).__name__, self._domain))

If that is what we want, I will, of course, adapt the doctest section.

[Matthias Köppe]

I haven't followed the discussion regarding restriction and equality, so I would need more context here.

[Michael Jung]

Replying to mkoeppe:

I haven't followed the discussion regarding restriction and equality, so I would need more context here.

Sorry, sure. See #30116 for the discussion.

Manifolds allows coercions from scalar fields on M to scalar fields on U, where U is a subset of M, i.e. a restriction. Since #30116, the equality check compares scalar fields on a common subset and returns True if they are the same on that domain. Nevertheless, both scalar fields on different domains are still distinct objects defined in distinct sets.

[Matthias Köppe]

You probably discussed this on that ticket, but this kind of "equality" relation is not transitive without strong assumptions such as analyticity and connectedness, is it?

[Michael Jung]

Replying to mkoeppe:

You probably discussed this on that ticket, but this kind of "equality" relation is not transitive without strong assumptions such as analyticity and connectedness, is it?

Actually, we didn't. But that is a crucial point! Without analyticity, which is not necessarily given, the transitivity is definitely violated.

However, coercible objects have to be comparable, right?

[Matthias Köppe]

In general I don't think you should feel obligated to use the coercion framework for comparisons just because you use it for arithmetic. It's just a tool.

[Michael Jung]

Replying to mkoeppe:

In general I don't think you should feel obligated to use the coercion framework for comparisons just because you use it for arithmetic. It's just a tool.

Probably you're right. The documentation, however, is not very explicit on that. I changed the ticket above to "needs info" and left a question. Perhaps you can take a short look?

[Michael Jung]

I just want to mention that your contributions are always very beneficial. Thank you very much! :)

[Matthias Köppe]

But soon I'll start bothering you with lots of naive questions about differential geometry... ;)

[Michael Jung]

Replying to mkoeppe:

But soon I'll start bothering you with lots of naive questions about differential geometry... ;)

That's only fair. ;)

[Michael Jung]

I will remove the dependence from #30116 again.

[git]

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

​37b0042

Trac #30266: remove dependency of #30116

[git]

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

​2f72e19

Trac #30266: if statement missing

[git]

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

​793c162

Trac #30266: hash function improved

[Michael Jung]

Here we go. Let's see what our friend the patchbot says...

[git]

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

​1912193

Trac #30266: doctest improved

[Michael Jung]

Patchbot is satisfied.