Opened 4 months ago

Closed 3 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) Commit: 6f0f537ac836a781b276141d3e34199f4dd5781b
Dependencies: Stopgaps:

Description (last modified by egourgoulhon)

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 4 months ago by gh-mjungmath

  • Description modified (diff)

comment:2 Changed 4 months ago by gh-mjungmath

  • Branch set to u/gh-mjungmath/inequality_test_mixed

comment:3 Changed 4 months ago by git

  • Commit set to 0203a6d96388bba0e583915354fe0900c1b9b80d

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

​0203a6dTrac #30108: doctest cleaned

comment:4 Changed 4 months ago by gh-mjungmath

  • Status changed from new to needs_review

comment:5 Changed 4 months ago by git

  • Commit changed from 0203a6d96388bba0e583915354fe0900c1b9b80d to 37ebad94b3a77f4dc6b43f82db26b238210d0f9e

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

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

comment:6 Changed 4 months ago by git

  • Commit changed from 37ebad94b3a77f4dc6b43f82db26b238210d0f9e to 3312d465a49eb75359bb46a18c58722d69f606a4

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

​3312d46Trac #30108: comment new doctest for documentation

comment:7 Changed 4 months ago by gh-mjungmath

  • Cc chapoton added

comment:8 Changed 4 months ago by gh-mjungmath

  • Status changed from needs_review to needs_work
Last edited 4 months ago by gh-mjungmath (previous) (diff)

comment:9 Changed 4 months ago by git

  • Commit changed from 3312d465a49eb75359bb46a18c58722d69f606a4 to 3eba4e13c69609daf63ec89005fc98b37cee2b11

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

​3eba4e1Trac #30108: != bug fixed

comment:10 Changed 4 months ago by gh-mjungmath

  • Description modified (diff)
  • Status changed from needs_work to needs_review

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

Last edited 4 months ago by gh-mjungmath (previous) (diff)

comment:11 Changed 4 months ago by git

  • Commit changed from 3eba4e13c69609daf63ec89005fc98b37cee2b11 to aa605ab62771d4e0e5c0282f15ee910bf210f966

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

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

comment:12 Changed 4 months ago by git

  • Commit changed from aa605ab62771d4e0e5c0282f15ee910bf210f966 to b7d9f475e9aff734e814a4607f20ba469d29c36d

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

​b7d9f47Trac #30108: wrong import

comment:13 follow-up: Changed 4 months ago by 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?

I have already opened a new ticket for this: ​https://trac.sagemath.org/ticket/30116

Last edited 4 months ago by gh-mjungmath (previous) (diff)

comment:14 Changed 4 months ago by git

  • Commit changed from b7d9f475e9aff734e814a4607f20ba469d29c36d to 0b93b22c6aeb12b08a9ab210dc16a1e58d3d6685

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

​0b93b22Trac #30108: unused import richcmp removed

comment:15 Changed 3 months ago by gh-mjungmath

  • Component changed from geometry to manifolds

comment:16 Changed 3 months ago by egourgoulhon

  • 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 3 months ago by egourgoulhon

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 3 months ago by git

  • Commit changed from 0b93b22c6aeb12b08a9ab210dc16a1e58d3d6685 to 4a396298d78588463356c01806c2095bf4ce1dda

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

​4a39629Trac #30108: use super()

comment:19 follow-up: Changed 3 months ago by gh-mjungmath

  • 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: Changed 3 months ago by 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.

comment:21 in reply to: ↑ 20 Changed 3 months ago by gh-mjungmath

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 3 months ago by egourgoulhon

  • Reviewers set to Eric Gourgoulhon
  • Status changed from needs_review to positive_review

Good to go!

comment:23 Changed 3 months ago by gh-mjungmath

Perfect. Thank you! :)

comment:24 Changed 3 months ago by git

  • 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:

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

comment:25 Changed 3 months ago by gh-mjungmath

  • Status changed from needs_review to positive_review

comment:26 Changed 3 months ago by vbraun

  • Branch changed from u/gh-mjungmath/inequality_test_mixed to 6f0f537ac836a781b276141d3e34199f4dd5781b
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.