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:  sage9.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: 
Description (last modified by )
This ticket adds the immutability and hashability for scalar fields.
See #30261.
Change History (45)
comment:1 Changed 2 years ago by
Branch:  → u/ghmjungmath/immutability_for_scalar_fields 

comment:2 Changed 2 years ago by
Authors:  → Michael Jung 

Cc:  Eric Gourgoulhon Travis Scrimshaw Matthias Köppe added 
Commit:  → aebadedd85d782e004a626f207df5422fee5a9ab 
Component:  PLEASE CHANGE → manifolds 
Description:  modified (diff) 
Keywords:  immutability added 
Status:  new → needs_review 
Summary:  immutability for scalar fields → Immutability for scalar fields 
Type:  PLEASE CHANGE → enhancement 
comment:3 Changed 2 years ago by
Dependencies:  → #30181 

comment:5 followup: 6 Changed 2 years ago by
Given that ScalarField_repr_
does not print any contents, this hash does not seem to be useful to distinguish nonequal 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 followups: 7 9 Changed 2 years ago by
Replying to mkoeppe:
Given that
ScalarField_repr_
does not print any contents, this hash does not seem to be useful to distinguish nonequal elements. This will degrade the performance of sets to that of lists.More importantly, it must be ensured that
f == g
implieshash(f) == hash(g)
. Because equality does not take names into account butrepr
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 followup: 8 Changed 2 years ago by
Replying to ghmjungmath:
Are you sure it is not
f is g
implieshash(f) == hash(g)
?
This implication is trivially true.
comment:8 Changed 2 years ago by
Replying to mkoeppe:
Replying to ghmjungmath:
Are you sure it is not
f is g
implieshash(f) == hash(g)
?This implication is trivially true.
Oh, yes, this is obviously right. >.< :D
comment:9 Changed 2 years ago by
Replying to ghmjungmath:
Replying to mkoeppe:
Given that
ScalarField_repr_
does not print any contents, this hash does not seem to be useful to distinguish nonequal elements. This will degrade the performance of sets to that of lists.More importantly, it must be ensured that
f == g
implieshash(f) == hash(g)
. Because equality does not take names into account butrepr
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
Commit:  aebadedd85d782e004a626f207df5422fee5a9ab → 5c7fd6e1933dd09daab91eda15df09572148413b 

Branch pushed to git repo; I updated commit sha1. New commits:
5c7fd6e  Trac #30266: immutability of restrictions + hash function improved

comment:12 Changed 2 years ago by
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
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
Commit:  5c7fd6e1933dd09daab91eda15df09572148413b → 8ca8b754a246ebad775b7db7310e4996f086687a 

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

comment:15 Changed 2 years ago by
Commit:  8ca8b754a246ebad775b7db7310e4996f086687a → 0bf9f36d692396f8aa2b32a529532c2e4e48511e 

Branch pushed to git repo; I updated commit sha1. New commits:
0bf9f36  Trac #30266: Merge branch 'develop' into t/30266/immutability_for_scalar_fields

comment:17 followup: 18 Changed 2 years ago by
I believe for _richcmp_
you also have self is not other
, but you should double check this.
comment:18 followup: 19 Changed 2 years ago by
comment:19 Changed 2 years ago by
Replying to ghmjungmath:
Replying to tscrim:
I believe for
_richcmp_
you also haveself 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 followup: 22 Changed 2 years ago by
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
Commit:  0bf9f36d692396f8aa2b32a529532c2e4e48511e → a5f6a19d0b492cbe2b73daf74e1a6b3a98667055 

Branch pushed to git repo; I updated commit sha1. New commits:
a5f6a19  Trac 30266: hash function slightly improved

comment:22 Changed 2 years ago by
Replying to ghmjungmath:
I changed the whole
_richcmp_
toreturn False
and triedf == f
. It returnedFalse
. 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
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
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?
comment:25 Changed 2 years ago by
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 followup: 27 Changed 2 years ago by
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 Changed 2 years ago by
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 followup: 29 Changed 2 years ago by
I haven't followed the discussion regarding restriction and equality, so I would need more context here.
comment:29 Changed 2 years ago by
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.
comment:30 followup: 31 Changed 2 years ago by
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 Changed 2 years ago by
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 followup: 33 Changed 2 years ago by
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 Changed 2 years ago by
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
I just want to mention that your contributions are always very beneficial. Thank you very much! :)
comment:35 followup: 36 Changed 2 years ago by
But soon I'll start bothering you with lots of naive questions about differential geometry... ;)
comment:36 Changed 2 years ago by
Replying to mkoeppe:
But soon I'll start bothering you with lots of naive questions about differential geometry... ;)
That's only fair. ;)
comment:38 Changed 2 years ago by
Commit:  a5f6a19d0b492cbe2b73daf74e1a6b3a98667055 → 37b004279233b59c9044e82fb1578a3025beb258 

Branch pushed to git repo; I updated commit sha1. New commits:
37b0042  Trac #30266: remove dependency of #30116

comment:39 Changed 2 years ago by
Commit:  37b004279233b59c9044e82fb1578a3025beb258 → 2f72e19ef4e48b1dd886a3b7ec3488e0e95b284e 

Branch pushed to git repo; I updated commit sha1. New commits:
2f72e19  Trac #30266: if statement missing

comment:40 Changed 2 years ago by
Commit:  2f72e19ef4e48b1dd886a3b7ec3488e0e95b284e → 793c162342e961c60ddc3ba704a59934d921c47d 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
793c162  Trac #30266: hash function improved

comment:41 Changed 2 years ago by
Dependencies:  #30181, #30116 → #30181 

Here we go. Let's see what our friend the patchbot says...
comment:42 Changed 2 years ago by
Commit:  793c162342e961c60ddc3ba704a59934d921c47d → 1912193c1c40222a7ce606e318fb1c5d5126cca4 

Branch pushed to git repo; I updated commit sha1. New commits:
1912193  Trac #30266: doctest improved

comment:44 Changed 2 years ago by
Reviewers:  → Matthias Koeppe 

Status:  needs_review → positive_review 
comment:45 Changed 2 years ago by
Branch:  u/ghmjungmath/immutability_for_scalar_fields → 1912193c1c40222a7ce606e318fb1c5d5126cca4 

Resolution:  → fixed 
Status:  positive_review → closed 
New commits:
FreeModuleTensor, Vector: Subclass a new class ModuleElementWithMutability
sage.tensor.modules: Make all zero() and one() elements immutable
FreeModuleTensor: Replace special casing of the immutable zero by is_immutable
ModuleElementWithMutability.is_immutable, is_mutable: Change def to cpdef
Trac #30266: make scalarfields immutable