Opened 12 months ago
Closed 11 months ago
#30108 closed defect (fixed)
Not equal comparison for mixed forms
Reported by:  ghmjungmath  Owned by:  

Priority:  major  Milestone:  sage9.2 
Component:  manifolds  Keywords:  mixed_forms, manifolds 
Cc:  egourgoulhon, tscrim, chapoton, mkoeppe  Merged in:  
Authors:  Michael Jung  Reviewers:  Eric Gourgoulhon 
Report Upstream:  N/A  Work issues:  
Branch:  6f0f537 (Commits, GitHub, GitLab)  Commit:  6f0f537ac836a781b276141d3e34199f4dd5781b 
Dependencies:  Stopgaps: 
Description (last modified by )
At this stage, the not equal comparison fails:
sage: M = Manifold(4, 'M') sage: X.<x,y,z,t> = M.chart() sage: Form0 = M.mixed_form(comp=([0,0,0,0,0])) sage: Form1 = M.mixed_form(comp=([1,0,0,0,0])) sage: Form0 != Form1 False
(reported at https://ask.sagemath.org/question/52409/whydoesntorworkformixedforms/)
I already spotted the error: originally, richcmp
is checked separately on each component and returns False
if the check fails for at least one element. But for the inequality, it is enough when the check succeeds for at least one element because then both mixed forms are already unequal.
This bug gets fixed in this ticket. Furthermore, an equality check between mixed forms and differential forms will be possible.
Change History (26)
comment:1 Changed 12 months ago by
 Description modified (diff)
comment:2 Changed 12 months ago by
 Branch set to u/ghmjungmath/inequality_test_mixed
comment:3 Changed 12 months ago by
 Commit set to 0203a6d96388bba0e583915354fe0900c1b9b80d
comment:4 Changed 12 months ago by
 Status changed from new to needs_review
comment:5 Changed 12 months ago by
 Commit changed from 0203a6d96388bba0e583915354fe0900c1b9b80d to 37ebad94b3a77f4dc6b43f82db26b238210d0f9e
Branch pushed to git repo; I updated commit sha1. New commits:
37ebad9  Trac #30108: doctest to check whether bug is fixed

comment:6 Changed 12 months ago by
 Commit changed from 37ebad94b3a77f4dc6b43f82db26b238210d0f9e to 3312d465a49eb75359bb46a18c58722d69f606a4
Branch pushed to git repo; I updated commit sha1. New commits:
3312d46  Trac #30108: comment new doctest for documentation

comment:7 Changed 12 months ago by
 Cc chapoton added
comment:8 Changed 12 months ago by
 Status changed from needs_review to needs_work
comment:9 Changed 12 months ago by
 Commit changed from 3312d465a49eb75359bb46a18c58722d69f606a4 to 3eba4e13c69609daf63ec89005fc98b37cee2b11
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
3eba4e1  Trac #30108: != bug fixed

comment:10 Changed 12 months ago by
 Description modified (diff)
 Status changed from needs_work to needs_review
I have overwritten my former approach. This should be much cleaner.
comment:11 Changed 12 months ago by
 Commit changed from 3eba4e13c69609daf63ec89005fc98b37cee2b11 to aa605ab62771d4e0e5c0282f15ee910bf210f966
Branch pushed to git repo; I updated commit sha1. New commits:
aa605ab  Trac #30108: eq check between scalar fields and mixed forms

comment:12 Changed 12 months ago by
 Commit changed from aa605ab62771d4e0e5c0282f15ee910bf210f966 to b7d9f475e9aff734e814a4607f20ba469d29c36d
Branch pushed to git repo; I updated commit sha1. New commits:
b7d9f47  Trac #30108: wrong import

comment:13 followup: ↓ 17 Changed 12 months ago by
For the sake of consistency, and compatibility, I think that the method __eq__
should be replaced by _richcmp_
in scalarfield.py
and tensorfield.py
. The method _richcmp_
tries to coerce both elements into elements of a common parent first which makes most of the current checks obsolete and increases compatibility.
Eric, what do you think?
I have already opened a new ticket for this: https://trac.sagemath.org/ticket/30116
comment:14 Changed 12 months ago by
 Commit changed from b7d9f475e9aff734e814a4607f20ba469d29c36d to 0b93b22c6aeb12b08a9ab210dc16a1e58d3d6685
Branch pushed to git repo; I updated commit sha1. New commits:
0b93b22  Trac #30108: unused import richcmp removed

comment:15 Changed 12 months ago by
 Component changed from geometry to manifolds
comment:16 Changed 11 months ago by
 Description modified (diff)
 Summary changed from Inequality comparison for mixed forms to Not equal comparison for mixed forms
comment:17 in reply to: ↑ 13 Changed 11 months ago by
Replying to ghmjungmath:
For the sake of consistency, and compatibility, I think that the method
__eq__
should be replaced by_richcmp_
inscalarfield.py
andtensorfield.py
. The method_richcmp_
tries to coerce both elements into elements of a common parent first which makes most of the current checks obsolete and increases compatibility.Eric, what do you think?
This sounds like a good idea, except maybe when efficiency matters, like for ManifoldPoint
, since points are parameters of UniqueRepresentation
objects, namely tangent spaces.
comment:18 Changed 11 months ago by
 Commit changed from 0b93b22c6aeb12b08a9ab210dc16a1e58d3d6685 to 4a396298d78588463356c01806c2095bf4ce1dda
Branch pushed to git repo; I updated commit sha1. New commits:
4a39629  Trac #30108: use super()

comment:19 followup: ↓ 20 Changed 11 months ago by
 Cc mkoeppe added
Can someone please review this ticket? There's not much change going on, and someone asked, rather indirectly, for a fix.
comment:20 in reply to: ↑ 19 ; followup: ↓ 21 Changed 11 months ago by
Replying to ghmjungmath:
Can someone please review this ticket? There's not much change going on, and someone asked, rather indirectly, for a fix.
LGTM. Just waiting for the patchbot green light.
comment:21 in reply to: ↑ 20 Changed 11 months ago by
Replying to egourgoulhon:
Replying to ghmjungmath:
Can someone please review this ticket? There's not much change going on, and someone asked, rather indirectly, for a fix.
LGTM. Just waiting for the patchbot green light.
Patchbot appears to be green now. Thank you.
comment:22 Changed 11 months ago by
 Reviewers set to Eric Gourgoulhon
 Status changed from needs_review to positive_review
Good to go!
comment:23 Changed 11 months ago by
Perfect. Thank you! :)
comment:24 Changed 11 months ago by
 Commit changed from 4a396298d78588463356c01806c2095bf4ce1dda to 6f0f537ac836a781b276141d3e34199f4dd5781b
 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:
6f0f537  Trac #30108: Merge branch 'develop' into t/30108/inequality_test_mixed

comment:25 Changed 11 months ago by
 Status changed from needs_review to positive_review
comment:26 Changed 11 months ago by
 Branch changed from u/ghmjungmath/inequality_test_mixed to 6f0f537ac836a781b276141d3e34199f4dd5781b
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
Trac #30108: doctest cleaned