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:

Status badges

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

  • Description modified (diff)

comment:2 Changed 23 months ago by gh-mjungmath

  • Branch set to u/gh-mjungmath/inequality_test_mixed

comment:3 Changed 23 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 23 months ago by gh-mjungmath

  • Status changed from new to needs_review

comment:5 Changed 23 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 23 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 23 months ago by gh-mjungmath

  • Cc chapoton added

comment:8 Changed 23 months ago by gh-mjungmath

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

comment:9 Changed 23 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 23 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 23 months ago by gh-mjungmath (previous) (diff)

comment:11 Changed 23 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 23 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 23 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 23 months ago by gh-mjungmath (previous) (diff)

comment:14 Changed 23 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 23 months ago by gh-mjungmath

  • Component changed from geometry to manifolds

comment:16 Changed 23 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 23 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 22 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 22 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 22 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 22 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 22 months ago by egourgoulhon

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

Good to go!

comment:23 Changed 22 months ago by gh-mjungmath

Perfect. Thank you! :)

comment:24 Changed 22 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 22 months ago by gh-mjungmath

  • Status changed from needs_review to positive_review

comment:26 Changed 22 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.