Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#26890 closed defect (fixed)

ElementWrapperCheckWrappedClass does not implement comparison properly

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-8.6
Component: categories Keywords:
Cc: Merged in:
Authors: Jeroen Demeyer Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 8418f8e (Commits, GitHub, GitLab) Commit: 8418f8eea1a2bf57ac1ea6df31585eac00496a9e
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

Comparisons with the wrapped class should just delegate to the wrapped class, handling also inequalities instead of only == and !=.

Change History (15)

comment:1 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 4 years ago by jdemeyer

  • Dependencies set to #26586

comment:3 Changed 4 years ago by jdemeyer

  • Branch set to u/jdemeyer/elementwrappercheckwrappedclass_does_not_implement_comparison_properly

comment:4 Changed 4 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Commit set to 8acc5866ac55ba107b979bbaafa09c29bd17de7a
  • Status changed from new to needs_review

New commits:

dc77918Mark doctest as known bug
8acc586Delegate comparisons for ElementWrapperCheckWrappedClass to wrapped class

comment:5 Changed 4 years ago by tscrim

You are changing the comparison logic a bit, in particular, comparisons between two wrapped classes no longer takes into account the parents. Since this is an extension class, it will not get subclassed when constructing elements; you've added an isinstance check that would also take care of if that happened as well. So we would get comparisons between objects that should not necessarily be treated as the same.

For a more concrete example, consider (1,0) as a Z-vector and (1,0) as a partition in an 2x2 box. These are (mathematically) incomparable objects, but your change would have them be equal. So at least the parent check should be added back in.

My recollection of why this was done this way was because we did not want the wrapped objects to have an ordering on them. However, I do not think there is any harm in inducing an ordering from the underlying objects. So the rest of your changes are fine with me.

comment:6 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:7 Changed 4 years ago by git

  • Commit changed from 8acc5866ac55ba107b979bbaafa09c29bd17de7a to ada617424e2350b4842ae5bdb3c6404dda7c9887

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

ada6174Delegate comparisons for ElementWrapperCheckWrappedClass to wrapped class

comment:8 Changed 4 years ago by git

  • Commit changed from ada617424e2350b4842ae5bdb3c6404dda7c9887 to 7c515f271abc7df392f6b4ea0c1347f2449fdcba

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

7c515f2Delegate comparisons for ElementWrapperCheckWrappedClass to wrapped class

comment:9 Changed 4 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:10 Changed 4 years ago by tscrim

  • Reviewers set to Travis Scrimshaw

There is one failure because of the fact that coercion is now taken into account. So we also need this change in sage/combinat/root_system/extended_affine_weyl_group.py:

     Note that for untwisted affine type the dual form of the classical Weyl group
     is isomorphic to the usual one, but acts on a different lattice and is therefore different to sage::
 
         sage: W0v = E.dual_classical_weyl(); W0v
         Weyl Group of type ['A', 2] (as a matrix group acting on the weight lattice)
         sage: v = W0v.from_reduced_word([1,2])
         sage: x = PvW0(v); x
         s1*s2
         sage: y = PW0(v); y
         s1*s2
-        sage: x == y
-        False
         sage: x.parent() == y.parent()
         False
+
+    However, because there is a coercion from ``PvW0`` to ``PW0``,
+    the elements ``x`` and ``y`` compare as equal::
+
+        sage: x == y
+        True
+

Once changed, you can set a positive review on my behalf.

comment:11 Changed 4 years ago by git

  • Commit changed from 7c515f271abc7df392f6b4ea0c1347f2449fdcba to 8418f8eea1a2bf57ac1ea6df31585eac00496a9e

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

8418f8eDelegate comparisons for ElementWrapperCheckWrappedClass to wrapped class

comment:12 Changed 4 years ago by jdemeyer

  • Dependencies #26586 deleted
  • Status changed from needs_review to positive_review

comment:13 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:14 Changed 4 years ago by vbraun

  • Branch changed from u/jdemeyer/elementwrappercheckwrappedclass_does_not_implement_comparison_properly to 8418f8eea1a2bf57ac1ea6df31585eac00496a9e
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:15 Changed 3 years ago by embray

  • Milestone changed from sage-8.5 to sage-8.6

This tickets were closed as fixed after the Sage 8.5 release.

Note: See TracTickets for help on using tickets.