Opened 23 months ago
Closed 22 months ago
#30108 closed defect (fixed)
Not equal comparison for mixed forms
Reported by: | gh-mjungmath | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-9.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/why-doesnt-or-work-for-mixed-forms/)
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 23 months ago by
- Description modified (diff)
comment:2 Changed 23 months ago by
- Branch set to u/gh-mjungmath/inequality_test_mixed
comment:3 Changed 23 months ago by
- Commit set to 0203a6d96388bba0e583915354fe0900c1b9b80d
comment:4 Changed 23 months ago by
- Status changed from new to needs_review
comment:5 Changed 23 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 23 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 23 months ago by
- Cc chapoton added
comment:8 Changed 23 months ago by
- Status changed from needs_review to needs_work
comment:9 Changed 23 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 23 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 23 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 23 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 follow-up: ↓ 17 Changed 23 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 23 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 23 months ago by
- Component changed from geometry to manifolds
comment:16 Changed 23 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 23 months ago by
Replying to gh-mjungmath:
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 22 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 follow-up: ↓ 20 Changed 22 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 ; follow-up: ↓ 21 Changed 22 months ago by
Replying to gh-mjungmath:
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 22 months ago by
Replying to egourgoulhon:
Replying to gh-mjungmath:
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 22 months ago by
- Reviewers set to Eric Gourgoulhon
- Status changed from needs_review to positive_review
Good to go!
comment:23 Changed 22 months ago by
Perfect. Thank you! :)
comment:24 Changed 22 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 22 months ago by
- Status changed from needs_review to positive_review
comment:26 Changed 22 months ago by
- Branch changed from u/gh-mjungmath/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