Opened 3 years ago

Closed 3 years ago

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

  • Commit set to 93020a4eb1213bfe866aba47ccaa60f8ee06a1d7

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

comment:2 Changed 3 years 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 3 years ago by embray

  • Status changed from new to needs_review

comment:4 Changed 3 years 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 3 years 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 3 years ago by jdemeyer

Just FYI: this conflicts with #19628

comment:7 in reply to: ↑ 5 Changed 3 years 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 3 years ago by jdemeyer

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

comment:9 in reply to: ↑ 6 ; follow-up: Changed 3 years 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 3 years ago by jdemeyer

Replying to embray:

Is it something I could be of help with?

It just needs review...

comment:11 Changed 3 years 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 3 years 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 3 years 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 3 years ago by chapoton

We should keep the test for !=

comment:15 Changed 3 years ago by embray

  • Status changed from needs_review to needs_work

That's a good point.

comment:16 Changed 3 years 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 3 years ago by chapoton

  • Keywords python3 richcmp added

comment:18 Changed 3 years ago by chapoton

is this back to needs_review ?

comment:19 Changed 3 years ago by chapoton

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

I will assume so.

comment:20 Changed 3 years 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 3 years 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 3 years ago by jdemeyer

What are "other comparisons" in that sentence?

comment:23 Changed 3 years ago by embray

< > <= >=?

comment:24 Changed 3 years ago by jdemeyer

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

comment:25 Changed 3 years ago by jdemeyer

I would just remove that sentence completely.

comment:26 Changed 3 years 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 3 years 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 3 years 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 3 years 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 3 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:31 Changed 3 years 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.