Opened 2 years ago

Closed 2 years ago

#30108 closed defect (fixed)

Not equal comparison for mixed forms

Reported by: Michael Jung Owned by:
Priority: major Milestone: sage-9.2
Component: manifolds Keywords: mixed_forms, manifolds
Cc: Eric Gourgoulhon, Travis Scrimshaw, Frédéric Chapoton, Matthias Köppe Merged in:
Authors: Michael Jung Reviewers: Eric Gourgoulhon
Report Upstream: N/A Work issues:
Branch: 6f0f537 (Commits, GitHub, GitLab) Commit: 6f0f537ac836a781b276141d3e34199f4dd5781b
Dependencies: Stopgaps:

Status badges

Description (last modified by Eric Gourgoulhon)

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 2 years ago by Michael Jung

Description: modified (diff)

comment:2 Changed 2 years ago by Michael Jung

Branch: u/gh-mjungmath/inequality_test_mixed

comment:3 Changed 2 years ago by git

Commit: 0203a6d96388bba0e583915354fe0900c1b9b80d

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

0203a6dTrac #30108: doctest cleaned

comment:4 Changed 2 years ago by Michael Jung

Status: newneeds_review

comment:5 Changed 2 years ago by git

Commit: 0203a6d96388bba0e583915354fe0900c1b9b80d37ebad94b3a77f4dc6b43f82db26b238210d0f9e

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

37ebad9Trac #30108: doctest to check whether bug is fixed

comment:6 Changed 2 years ago by git

Commit: 37ebad94b3a77f4dc6b43f82db26b238210d0f9e3312d465a49eb75359bb46a18c58722d69f606a4

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

3312d46Trac #30108: comment new doctest for documentation

comment:7 Changed 2 years ago by Michael Jung

Cc: Frédéric Chapoton added

comment:8 Changed 2 years ago by Michael Jung

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

comment:9 Changed 2 years ago by git

Commit: 3312d465a49eb75359bb46a18c58722d69f606a43eba4e13c69609daf63ec89005fc98b37cee2b11

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

3eba4e1Trac #30108: != bug fixed

comment:10 Changed 2 years ago by Michael Jung

Description: modified (diff)
Status: needs_workneeds_review

I have overwritten my former approach. This should be much cleaner.

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

comment:11 Changed 2 years ago by git

Commit: 3eba4e13c69609daf63ec89005fc98b37cee2b11aa605ab62771d4e0e5c0282f15ee910bf210f966

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

aa605abTrac #30108: eq check between scalar fields and mixed forms

comment:12 Changed 2 years ago by git

Commit: aa605ab62771d4e0e5c0282f15ee910bf210f966b7d9f475e9aff734e814a4607f20ba469d29c36d

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

b7d9f47Trac #30108: wrong import

comment:13 Changed 2 years ago by Michael Jung

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

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

comment:14 Changed 2 years ago by git

Commit: b7d9f475e9aff734e814a4607f20ba469d29c36d0b93b22c6aeb12b08a9ab210dc16a1e58d3d6685

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

0b93b22Trac #30108: unused import richcmp removed

comment:15 Changed 2 years ago by Michael Jung

Component: geometrymanifolds

comment:16 Changed 2 years ago by Eric Gourgoulhon

Description: modified (diff)
Summary: Inequality comparison for mixed formsNot equal comparison for mixed forms

comment:17 in reply to:  13 Changed 2 years ago by Eric Gourgoulhon

Replying to gh-mjungmath:

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?

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 2 years ago by git

Commit: 0b93b22c6aeb12b08a9ab210dc16a1e58d3d66854a396298d78588463356c01806c2095bf4ce1dda

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

4a39629Trac #30108: use super()

comment:19 Changed 2 years ago by Michael Jung

Cc: Matthias Köppe 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 ; Changed 2 years ago by Eric Gourgoulhon

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 2 years ago by Michael Jung

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 2 years ago by Eric Gourgoulhon

Reviewers: Eric Gourgoulhon
Status: needs_reviewpositive_review

Good to go!

comment:23 Changed 2 years ago by Michael Jung

Perfect. Thank you! :)

comment:24 Changed 2 years ago by git

Commit: 4a396298d78588463356c01806c2095bf4ce1dda6f0f537ac836a781b276141d3e34199f4dd5781b
Status: positive_reviewneeds_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

6f0f537Trac #30108: Merge branch 'develop' into t/30108/inequality_test_mixed

comment:25 Changed 2 years ago by Michael Jung

Status: needs_reviewpositive_review

comment:26 Changed 2 years ago by Volker Braun

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