Opened 2 years ago

Closed 2 years ago

#30266 closed enhancement (fixed)

Immutability for scalar fields

Reported by: Michael Jung Owned by:
Priority: major Milestone: sage-9.2
Component: manifolds Keywords: immutability
Cc: Eric Gourgoulhon, Travis Scrimshaw, Matthias Köppe Merged in:
Authors: Michael Jung Reviewers: Matthias Koeppe
Report Upstream: N/A Work issues:
Branch: 1912193 (Commits, GitHub, GitLab) Commit: 1912193c1c40222a7ce606e318fb1c5d5126cca4
Dependencies: #30181 Stopgaps:

Status badges

Description (last modified by Michael Jung)

This ticket adds the immutability and hashability for scalar fields.

See #30261.

Change History (45)

comment:1 Changed 2 years ago by Michael Jung

Branch: u/gh-mjungmath/immutability_for_scalar_fields

comment:2 Changed 2 years ago by Michael Jung

Authors: Michael Jung
Cc: Eric Gourgoulhon Travis Scrimshaw Matthias Köppe added
Commit: aebadedd85d782e004a626f207df5422fee5a9ab
Component: PLEASE CHANGEmanifolds
Description: modified (diff)
Keywords: immutability added
Status: newneeds_review
Summary: immutability for scalar fieldsImmutability for scalar fields
Type: PLEASE CHANGEenhancement

New commits:

286d3c6FreeModuleTensor, Vector: Subclass a new class ModuleElementWithMutability
9df3d22sage.tensor.modules: Make all zero() and one() elements immutable
4373ea2FreeModuleTensor: Replace special casing of the immutable zero by is_immutable
a2ee8beModuleElementWithMutability.is_immutable, is_mutable: Change def to cpdef
aebadedTrac #30266: make scalarfields immutable

comment:3 Changed 2 years ago by Michael Jung

Dependencies: #30181

comment:4 Changed 2 years ago by Michael Jung

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

comment:5 Changed 2 years ago by 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.

comment:6 in reply to:  5 ; Changed 2 years ago by 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?

comment:7 in reply to:  6 ; Changed 2 years ago by 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.

comment:8 in reply to:  7 Changed 2 years ago by 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

comment:9 in reply to:  6 Changed 2 years ago by 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.

comment:10 Changed 2 years ago by git

Commit: aebadedd85d782e004a626f207df5422fee5a9ab5c7fd6e1933dd09daab91eda15df09572148413b

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

5c7fd6eTrac #30266: immutability of restrictions + hash function improved

comment:11 Changed 2 years ago by Michael Jung

This should be better.

comment:12 Changed 2 years ago by Michael Jung

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

Patchbot is green, btw.

comment:13 Changed 2 years ago by Michael Jung

Dependencies: #30181#30181, #30116

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.

comment:14 Changed 2 years ago by git

Commit: 5c7fd6e1933dd09daab91eda15df09572148413b8ca8b754a246ebad775b7db7310e4996f086687a

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

comment:15 Changed 2 years ago by git

Commit: 8ca8b754a246ebad775b7db7310e4996f086687a0bf9f36d692396f8aa2b32a529532c2e4e48511e

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

0bf9f36Trac #30266: Merge branch 'develop' into t/30266/immutability_for_scalar_fields

comment:16 Changed 2 years ago by Michael Jung

Patchbot is green.

comment:17 Changed 2 years ago by Travis Scrimshaw

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

comment:18 in reply to:  17 ; Changed 2 years ago by 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?

comment:19 in reply to:  18 Changed 2 years ago by 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.

comment:20 Changed 2 years ago by 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.

comment:21 Changed 2 years ago by git

Commit: 0bf9f36d692396f8aa2b32a529532c2e4e48511ea5f6a19d0b492cbe2b73daf74e1a6b3a98667055

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

a5f6a19Trac 30266: hash function slightly improved

comment:22 in reply to:  20 Changed 2 years ago by 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. :)

comment:23 Changed 2 years ago by 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.

comment:24 Changed 2 years ago by 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?

Last edited 2 years ago by Michael Jung (previous) (diff)

comment:25 Changed 2 years ago by 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

comment:26 Changed 2 years ago by 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.

comment:27 in reply to:  26 Changed 2 years ago by 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.

comment:28 Changed 2 years ago by Matthias Köppe

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

comment:29 in reply to:  28 Changed 2 years ago by 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.

Last edited 2 years ago by Michael Jung (previous) (diff)

comment:30 Changed 2 years ago by 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?

comment:31 in reply to:  30 Changed 2 years ago by 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?

comment:32 Changed 2 years ago by 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.

comment:33 in reply to:  32 Changed 2 years ago by 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?

comment:34 Changed 2 years ago by Michael Jung

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

comment:35 Changed 2 years ago by Matthias Köppe

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

comment:36 in reply to:  35 Changed 2 years ago by Michael Jung

Replying to mkoeppe:

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

That's only fair. ;)

comment:37 Changed 2 years ago by Michael Jung

I will remove the dependence from #30116 again.

comment:38 Changed 2 years ago by git

Commit: a5f6a19d0b492cbe2b73daf74e1a6b3a9866705537b004279233b59c9044e82fb1578a3025beb258

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

37b0042Trac #30266: remove dependency of #30116

comment:39 Changed 2 years ago by git

Commit: 37b004279233b59c9044e82fb1578a3025beb2582f72e19ef4e48b1dd886a3b7ec3488e0e95b284e

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

2f72e19Trac #30266: if statement missing

comment:40 Changed 2 years ago by git

Commit: 2f72e19ef4e48b1dd886a3b7ec3488e0e95b284e793c162342e961c60ddc3ba704a59934d921c47d

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

793c162Trac #30266: hash function improved

comment:41 Changed 2 years ago by Michael Jung

Dependencies: #30181, #30116#30181

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

comment:42 Changed 2 years ago by git

Commit: 793c162342e961c60ddc3ba704a59934d921c47d1912193c1c40222a7ce606e318fb1c5d5126cca4

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

1912193Trac #30266: doctest improved

comment:43 Changed 2 years ago by Michael Jung

Patchbot is satisfied.

comment:44 Changed 2 years ago by Matthias Köppe

Reviewers: Matthias Koeppe
Status: needs_reviewpositive_review

comment:45 Changed 2 years ago by Volker Braun

Branch: u/gh-mjungmath/immutability_for_scalar_fields1912193c1c40222a7ce606e318fb1c5d5126cca4
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.