#24791 closed defect (fixed)

py3: fix tests in sage.structure.unique_representation

Reported by: embray Owned by:
Priority: minor Milestone: sage-8.2
Component: python3 Keywords: python3, richcmp
Cc: Merged in:
Authors: Erik Bray Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 62e7b05 (Commits) Commit: 62e7b05f4c53a6fd4ad311ea33891aaebdb670a1
Dependencies: Stopgaps:

Description

On one hand, we might still want to test __cmp__ with this code on Python 2. On the other hand, I think we'll be replacing most/all __cmp__ methods anyways, so maybe it doesn't matter. The comparison implementation itself is also not critical to how UniqueRepresentation works.

I'm fine with either way.

Change History (31)

comment:1 Changed 22 months ago by git

  • Commit set to 93020a4eb1213bfe866aba47ccaa60f8ee06a1d7

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

comment:2 Changed 22 months ago by git

  • Commit changed from 93020a4eb1213bfe866aba47ccaa60f8ee06a1d7 to 011250b1440e3de0af94cdd0046156be1ea6904c

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

011250bpy3: fix tests for sage.structure.unique_representation

comment:3 Changed 22 months ago by embray

  • Status changed from new to needs_review

comment:4 Changed 22 months ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to needs_work

This code looks strange:

        ....:         if op in (op_EQ, op_NE):
        ....:             if type(self) != type(other):
        ....:                 return op == op_NE

I wouldn't write that in an example. Would it work to do

        ....:         if type(self) is not type(other):
        ....:             return NotImplemented

comment:5 follow-up: Changed 22 months ago by jdemeyer

The docs are no longer consistent with the example. I actually think it's best to just remove the __cmp__ completely (instead of replacing it with __richcmp__) and then also remove the talk about custom comparison methods from the docs.

comment:6 follow-up: Changed 22 months ago by jdemeyer

Just FYI: this conflicts with #19628

comment:7 in reply to: ↑ 5 Changed 22 months ago by embray

Replying to jdemeyer:

The docs are no longer consistent with the example. I actually think it's best to just remove the __cmp__ completely (instead of replacing it with __richcmp__) and then also remove the talk about custom comparison methods from the docs.

That did seem out of place to me as well. Beyond __eq__ the talk of other comparison operators doesn't seem relevant, but I wasn't sure.

To be clear, you're just talking about the second example, and ensuing discussion of comparison methods, right? In the first example the __eq__, at least, is still necessary.

I ask just because, while I understand what this class does on its own, I wasn't sure if there was some subtlety of how it's used in Sage that necessitated this documentation.

comment:8 Changed 22 months ago by jdemeyer

Yes, I was talking about the example with the "custom cmp"

comment:9 in reply to: ↑ 6 ; follow-up: Changed 22 months ago by embray

Replying to jdemeyer:

Just FYI: this conflicts with #19628

That ticket looks like it's been sitting around for quite a while. Is it something I could be of help with?

comment:10 in reply to: ↑ 9 Changed 22 months ago by jdemeyer

Replying to embray:

Is it something I could be of help with?

It just needs review...

comment:11 Changed 22 months ago by git

  • Commit changed from 011250b1440e3de0af94cdd0046156be1ea6904c to 0d8c9637ae85fcdf02bbd801b6113cf8f3d7fda5

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

866edddWithEqualityById.__richcmp__: bail out if "other" is no instance of WithEqualityById
0d8c963Merge branch 'u/jdemeyer/lazy_import_breaks_cachedrepresentation' into u/embray/python3/sage-structure-unique_representation

comment:12 Changed 22 months ago by git

  • Commit changed from 0d8c9637ae85fcdf02bbd801b6113cf8f3d7fda5 to df224c66a5e0956682b469cbaedd4a897297b6c1

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

df224c6Simplify these docs by simply removing the custom comparison examples.

comment:13 Changed 22 months ago by embray

  • Dependencies set to #19628
  • Status changed from needs_work to needs_review

Merged in #19628 and removed the custom comparison examples. In the end the only change needed for Python 3 is to replace the one __cmp__ in the example for CachedRepresentation with __eq__, which makes more sense anyways.

comment:14 Changed 21 months ago by chapoton

We should keep the test for !=

comment:15 Changed 21 months ago by embray

  • Status changed from needs_review to needs_work

That's a good point.

comment:16 Changed 21 months ago by git

  • Commit changed from df224c66a5e0956682b469cbaedd4a897297b6c1 to 85ea4146a2ff9c7e9e92eb2ab79414d9501be0f9

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

85ea414Remove the needlessly confusing combination of two tests on one line; restore the != test since it is still relevant here

comment:17 Changed 21 months ago by chapoton

  • Keywords python3 richcmp added

comment:18 Changed 21 months ago by chapoton

is this back to needs_review ?

comment:19 Changed 21 months ago by chapoton

  • Dependencies #19628 deleted
  • Status changed from needs_work to needs_review

I will assume so.

comment:20 Changed 21 months ago by jdemeyer

  • Status changed from needs_review to needs_work

This sentence still makes no sense to me (see 5):

For other
comparisons, the relevant custom comparison is called if defined for either
the left-hand side or the right-hand side::

comment:21 Changed 21 months ago by embray

  • Status changed from needs_work to needs_review

Could you clarify? I think the sentence itself makes sense; or is at least true. If anything doesn't make sense it might just be the placement. I.e. the examples that immediately follow that sentence isn't really relevant (the sentence is more a modification to the previous sentence, to which the examples due apply).

Maybe if it were moved later? I wouldn't outright remove it--the point is just to say that other custom comparisons may still be implemented on classes that subclass from this.

comment:22 Changed 21 months ago by jdemeyer

What are "other comparisons" in that sentence?

comment:23 Changed 21 months ago by embray

< > <= >=?

comment:24 Changed 21 months ago by jdemeyer

OK, but then what is "the relevant custom comparison"?

comment:25 Changed 21 months ago by jdemeyer

I would just remove that sentence completely.

comment:26 Changed 21 months ago by embray

Okay... I don't really see a problem with it other than what I explained above, but I can certainly live without it as well.

comment:27 follow-up: Changed 21 months ago by jdemeyer

If that sentence is meant to say that you can subclass CachedRepresentation and add your own rich comparison methods, it's saying that very badly. It's also superfluous since that's how subclassing in Python works in general.

comment:28 Changed 21 months ago by git

  • Commit changed from 85ea4146a2ff9c7e9e92eb2ab79414d9501be0f9 to 62e7b05f4c53a6fd4ad311ea33891aaebdb670a1

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

62e7b05Some more minor edits, hopefully for clarity...

comment:29 in reply to: ↑ 27 Changed 21 months ago by embray

Replying to jdemeyer:

It's also superfluous since that's how subclassing in Python works in general.

I agree that it's superfluous, but so is a lot of the other text in there. I figured it was worth pointing out for those who aren't completely comfortable how subclassing works w.r.t. comparisons (heck, I'm not even completely comfortable with it, if recent experience dictates).

So it's saying "these objects have this specific type of behavior when it comes to identity comparisons, but may still have any other behavior w.r.t. ordering behavior". But I don't think it's too important to say that either.

comment:30 Changed 21 months ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:31 Changed 21 months ago by vbraun

  • Branch changed from u/embray/python3/sage-structure-unique_representation to 62e7b05f4c53a6fd4ad311ea33891aaebdb670a1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.