Opened 6 years ago

Closed 4 years ago

#19628 closed defect (fixed)

lazy_import breaks CachedRepresentation

Reported by: cheuberg Owned by:
Priority: major Milestone: sage-8.0
Component: coercion Keywords:
Cc: vbraun, jdemeyer, mmezzaroba, SimonKing, nthiery Merged in:
Authors: Jeroen Demeyer Reviewers: Erik Bray
Report Upstream: N/A Work issues:
Branch: 866eddd (Commits, GitHub, GitLab) Commit: 866eddd0d1982a0327ac37daf29bab857f227fa5
Dependencies: #23102 Stopgaps:

Status badges

Description (last modified by jdemeyer)

NN is a lazy_import and therefore we have

sage: NonNegativeIntegerSemiring() == NN
False

but

sage: NN == NonNegativeIntegerSemiring()
True
sage: NonNegativeIntegerSemiring() == NN._get_object()
True

This gives problems with CachedRepresentation:

sage: from sage.misc.lazy_import import LazyImport
sage: lazyZZ = LazyImport('sage.rings.integer_ring', 'ZZ')
sage: PolynomialRing(lazyZZ, 'x') is PolynomialRing(ZZ, 'x')
False

Change History (43)

comment:1 Changed 6 years ago by cheuberg

The attached branch is the one which exhibits the error.

comment:2 Changed 6 years ago by jdemeyer

  • Description modified (diff)

comment:3 Changed 6 years ago by jdemeyer

  • Component changed from numerical to coercion
  • Description modified (diff)
  • Summary changed from Fix interplay between lazy_import, coercion and arb to lazy_import breaks creation of polynomial rings

comment:4 Changed 6 years ago by jdemeyer

  • Dependencies #19152 deleted

comment:5 Changed 6 years ago by jdemeyer

  • Description modified (diff)
  • Summary changed from lazy_import breaks creation of polynomial rings to lazy_import breaks uniqueness of dict keys

comment:6 Changed 6 years ago by jdemeyer

  • Description modified (diff)
  • Summary changed from lazy_import breaks uniqueness of dict keys to Comparison fails for lazy_imports

comment:7 Changed 6 years ago by jdemeyer

  • Summary changed from Comparison fails for lazy_imports to lazy_import breaks UniqueRepresentation

comment:8 Changed 6 years ago by jdemeyer

  • Description modified (diff)

comment:9 Changed 6 years ago by jdemeyer

  • Cc SimonKing added

Should we special-case LazyImport in WithEqualityById?

comment:10 Changed 6 years ago by jdemeyer

  • Description modified (diff)

comment:11 Changed 6 years ago by jdemeyer

  • Description modified (diff)

comment:12 follow-up: Changed 6 years ago by nbruin

There are many subtle problems with lazy_import and some of them are fairly fundamental. The basic take-away is: don't lazy_import objects, but lazy_import functions that produce objects (NN is problematic but NonNegativeIntegerSemiring() is not).

LazyImport objects try to be as transparent as possible, but obviously won't succeed perfectly. They try to remove themselves once non-trivially accessed (that is, basically when one of their attributes gets accessed), and essentially LazyImport? objects should only be used to look up attributes on.

Many of our LazyImport objects get further imported erroneously. For instance, importing a LazyImport object from elsewhere via a from ... import <LazyImportObject> doesn't work properly, because the LazyImport? object doesn't get an opportunity to know about the new namespace in which it gets imported. An example is NN as referenced here.

If we insert a little helper function into sage.misc.lazy_import (it has to be there because lazy_import doesn't offer a pxd):

def att(a):
    cdef LazyImport b
    b = a
    return {"_object": b._object,
            "_module": b._module,
            "_name": b._name,
            "_as_name": b._as_name,
            "_namespace": b._namespace,
            "_at_startup": b._at_startup,
            "_deprecation": b._deprecation}

then you can see from

sage: sage.misc.lazy_import.att(NN)['_namespace']['__name__']
'sage.rings.semirings.all'
sage: sage.misc.lazy_import.att(NN)['_name']
'NN'
sage: sage.misc.lazy_import.att(NN)['_as_name']
'NN'

that accessing NN in the toplevel scope will never resolve. On every access, the LazyImport object will dutifully search the sage.rings.semirings.all scope to replace its occurrence there (which only happens the first time of course), but that's not the binding through which it gets accessed afterwards.

If instead you do (and this is how NN should be imported into the toplevel if at all):

sage: lazy_import(sage.misc.lazy_import.att(NN)['_module'], sage.misc.lazy_import.att(NN)['_name'])
sage: type(NN)
<type 'sage.misc.lazy_import.LazyImport'>
sage: NN
Non negative integer semiring
sage: type(NN)
<class 'sage.rings.semirings.non_negative_integer_semiring.NonNegativeIntegerSemiring_with_category'>

As you can see, basically even

from sage.rings.semirings.non_negative_integer_semiring import NN

is an erroneous use of a LazyImport object, because it does not amount to attribute lookup.

The reason why mathematically interesting objects themselves should probably not be lazily imported, but only their access functions is because

sage: { NN : 1 }

will always be problematic: The LazyImport? object doesn't get a chance to remove itself.

On the other hand, accessing the object via NonNegativeIntegerSemiring() works nicely.

Of course, NonNegativeIntegerSemiring itself is incorrectly imported.

We could clean up scopes (but that should happen in basically all __all__ files etc.) via something along the lines of:

scope=globals()
for name,value in scope.iteritems():
    if isinstance(value,sage.misc.lazy_import.LazyImport):
        T= sage.misc.lazy_import.att(value)
        if T['_namespace'] is not scope:
            scope[name] = sage.misc.lazy_import.LazyImport(T['_module'],T['_name'],T['_as_name'],scope,T['_at_startup'],T['_deprecation'])

Rather than hobbling EqualityById? to accommodate for the hack that is LazyImport, we should avoid erroneous use of LazyImport. The problem with EqualityById is just a symptom of a general problem. There are many tickets about this, see e.g. #12482

comment:13 in reply to: ↑ 12 ; follow-up: Changed 6 years ago by jdemeyer

  • Description modified (diff)
  • Summary changed from lazy_import breaks UniqueRepresentation to lazy_import breaks CachedRepresentation

Replying to nbruin:

We could clean up scopes (but that should happen in basically all __all__ files etc.)

Of course, now the question becomes: which scopes? If you can hand me a list of dicts which contain all relevant lazy imports, we can indeed do something.

There are many tickets about this, see e.g. #12482

Thanks for the pointer, I fixed this.

comment:14 Changed 6 years ago by jdemeyer

One thing which we could easily do is to change cached_function() and friends to automatically dereference lazy imports. This will fix at least the problem with CachedRepresentation I think.

comment:15 Changed 6 years ago by jdemeyer

  • Branch u/cheuberg/19152-arb-misc-lazy-import deleted
  • Commit b21aa6e069182b08aca3cf7c24d6ed24e5926510 deleted

comment:16 follow-up: Changed 6 years ago by vbraun

Still if we agree that it can't be done safely then maybe it's a bad idea?

An alternative might be to have a proxy object that, instead of trying to look like the original object, relies on the preparser to replace it? E.g. the preparser replaces NN with NonNegativeIntegerSemiring(). And library code would be forced to import from the original location.

Last edited 6 years ago by jdemeyer (previous) (diff)

comment:17 Changed 6 years ago by vbraun

  • Cc nthiery added

comment:18 in reply to: ↑ 16 ; follow-ups: Changed 6 years ago by jdemeyer

Replying to vbraun:

Still if we agree that it can't be done safely then maybe it's a bad idea?

What do the two "it"s refer to in this sentence?

Fixing cached functions can be done safely. We already pre-process arguments to cached functions in src/sage/misc/function_mangling.pyx, so it's not hard to also dereference lazy imports there. Of course this does not fix every single use-case of lazy imports, but this is still an important use-case.

Automatically "fixing" lazy imports in globals()-like dicts can be done, but there the hard problem is to determine which dicts should be processed.

An alternative might be to have a proxy object that, instead of trying to look like the original object, relies on the preparser to replace it? E.g. the preparser replaces NN with NonNegativeIntegerSemiring().

I'm not convinced that we want to involve the preparser for this. What if somebody writes

sage: NN = some_completely_unrelated_object
sage: f(NN)

you don't want this to be replaced by

sage: NonNegativeIntegerSemiring() = some_completely_unrelated_object
sage: f(NonNegativeIntegerSemiring())

comment:19 in reply to: ↑ 13 Changed 6 years ago by nbruin

Replying to jdemeyer:

Of course, now the question becomes: which scopes? If you can hand me a list of dicts which contain all relevant lazy imports, we can indeed do something.

All scopes that from ... import a symbol that is a LazyImport. That includes many "all" files. Each of those should basically finish with a

sage.misc.lazy_import.fix_sloppily_constructed_namespace(globals())

(and then we should hope that no fancy shenanigans have happened with the bindings during module initialization)

We should consider how badly this affects startup times. In time-critical bits, it may be better to use a fresh lazy_import command instead instead of fixing the namespace afterwards.

One could run this process on [m for m in sys.modules().itervalues() if m is not None] (there are quite some entries in there that are None. I don't know what they're doing there. Vanilla python has that too), but we'd really be meddling with data that isn't ours, so I'd expect that to lead to more fragileness.

The namespaces registered under sys.modules() would certainly provide a good starting point for assessing the scale of the problem.

comment:20 in reply to: ↑ 18 Changed 6 years ago by vbraun

Replying to jdemeyer:

What if somebody writes

sage: NN = some_completely_unrelated_object
sage: f(NN)

That would be perfectly fine. The preparser can't be a regex but has to look at the AST, of course. IPython already has support for that. Its also easier to implement since it leverages the Python parser.

comment:21 in reply to: ↑ 18 ; follow-up: Changed 6 years ago by vbraun

Replying to jdemeyer:

Still if we agree that it can't be done safely then maybe it's a bad idea?

What do the two "it"s refer to in this sentence?

Both "it" refer to "import of any object for which 'is'-comparison matters". As opposed to factory functions.

comment:22 Changed 6 years ago by nbruin

OK, and these are the counts for lazy_import objects bound in scopes that is not theirs:

from sage.misc.lazy_import import LazyImport,att #that's the helper function
def misbound_lazies(S):
  return [k for k,v in S.iteritems() if type(v) is LazyImport and att(v)['_namespace'] is not S]
M=[(k,len(misbound_lazies(m.__dict__))) for k,m in sys.modules.iteritems() if m is not None]
sage: [m for m in M if m[1] > 0]
[('sage.combinat.tableau_tuple', 1),
 ('sage.libs.linbox.linbox', 1),
 ('sage.groups.libgap_mixin', 1),
 ('__main__', 190),
 ('sage.categories.all', 1),
 ('sage.combinat.all', 34),
 ('sage.calculus.all', 1),
 ('sage.geometry.all', 3),
 ('sage.functions.piecewise', 1),
 ('sage.structure.sage_object', 1),
 ('sage.combinat.partition_tuple', 1),
 ('sage.modular.arithgroup.congroup_generic', 1),
 ('sage.schemes.all', 6),
 ('sage.rings.all', 3),
 ('sage.functions.special', 1),
 ('sage.functions.airy', 1),
 ('sage.all_cmdline', 190),
 ('sage.libs.pari.gen_py', 4),
 ('sage.structure.coerce', 1),
 ('sage.functions.bessel', 1),
 ('sage.functions.orthogonal_polys', 1),
 ('sage.numerical.interactive_simplex_method', 1),
 ('sage.combinat.integer_vectors_mod_permgroup', 1),
 ('sage.groups.all', 10),
 ('sage.combinat.crystals.tensor_product', 1),
 ('sage.combinat.composition', 1),
 ('sage.categories.groups', 1),
 ('sage.combinat.partition', 1),
 ('sage.all', 165)]
sage: len(set(flatten([misbound_lazies(m.__dict__) for m in sys.modules.values() if m is not None])))
195

So, assuming that symbol name is a good indication, there seem to be about 195 distinct lazy_import objects around that are bound in wrong scopes.

Most frequently occurring:

sage: L=[misbound_lazies(m.__dict__) for m in sys.modules.values() if m is not None]
sage: sorted(Counter(flatten(L)).items(),key=lambda a:a[1])[-20:]
[('WehlerK3Surface', 4),
 ('ClusterQuiver', 4),
 ('CrystalOfNakajimaMonomials', 4),
 ('KostkaFoulkesPolynomial', 4),
 ('polytopes', 4),
 ('ExtendedAffineWeylGroup', 4),
 ('QuaternionMatrixGroupGF3', 4),
 ('KirillovReshetikhinTableaux', 4),
 ('CrystalOfGeneralizedYoungWalls', 4),
 ('maxima_calculus', 4),
 ('AffineCrystalFromClassical', 4),
 ('ClusterSeed', 4),
 ('QuiverMutationType', 4),
 ('maxima', 4),
 ('NonNegativeIntegerSemiring', 5),
 ('Polyhedron', 5),
 ('MatrixGroup', 5),
 ('AsymptoticRing', 5),
 ('cartesian_product', 7),
 ('NN', 9)]

comment:23 in reply to: ↑ 21 ; follow-up: Changed 6 years ago by jdemeyer

Replying to vbraun:

Both "it" refer to "import of any object for which 'is'-comparison matters". As opposed to factory functions.

Now the question becomes: to what extent does equality matter? One of my proposals is to fix lazy imports in cached functions. That would seriously reduce the extent to which equality matters. It would certainly fix this:

sage: from sage.misc.lazy_import import LazyImport
sage: lazyZZ = LazyImport('sage.rings.integer_ring', 'ZZ')
sage: PolynomialRing(lazyZZ, 'x') is PolynomialRing(ZZ, 'x')
False

What do you think about this?

comment:24 in reply to: ↑ 23 ; follow-ups: Changed 6 years ago by nbruin

Replying to jdemeyer:

Now the question becomes: to what extent does equality matter? One of my proposals is to fix lazy imports in cached functions. That would seriously reduce the extent to which equality matters. It would certainly fix this:

sage: from sage.misc.lazy_import import LazyImport
sage: lazyZZ = LazyImport('sage.rings.integer_ring', 'ZZ')
sage: PolynomialRing(lazyZZ, 'x') is PolynomialRing(ZZ, 'x')
False

What do you think about this?

I know that Robert Bradshaw did not endorse the use of LazyImport for imports like this. The intended usage scenario was really just to replace "import" with a few extensions to support "from .. import .." for functions/classes/submodules. He was fully aware that the shim that LazyImport objects provide is imperfect. In his opinion, you shouldn't import ZZ this way. You can lazy-import sage.rings.integer_ring as integer_ring and then address ZZ as integer_ring.ZZ.

You can of course try to extend the use of lazy_import and see if you can get it to work for things like NN as well. However, you'll find (as some examples have shown) that it's always fundamentally problematic. I think we're better off rooting out the "bad use" than trying to (imperfectly) support it. It would be nice to support lazy importing like this fully, but I think it would require hacking python to an uncomfortable degree.

The problem isn't huge: I think NN is the worst symbol like this. Perhaps we can just not lazily import it? or otherwise make it accessible via an accessor function or via a lazily imported namespace (rather than importing NN lazily itself).

For a pretty complete tally: (so we're talking about 291 objects)

sage: def lazy_names(S):
....:      return [k for k,v in S.iteritems() if type(v) is LazyImport]
sage: L=[lazy_names(m.__dict__) for m in sys.modules.values() if m is not None]
sage: len(Counter(flatten(L)))
291

There are a lot of interesting bindings there, though:

sage: type(sage.misc.misc.SAGE_ROOT)
<type 'sage.misc.lazy_import.LazyImport'>

To see the types of these lazy import objects:

from sage.misc.lazy_import import LazyImport,att
from collections import Counter
def lazy_objects(S):
  return [v for k,v in S.iteritems() if type(v) is LazyImport]

L=[lazy_objects(m.__dict__) for m in sys.modules.values() if m is not None]
O=[]
for l in L: O.extend(l)
%cpaste
for o in O: #try to load all objects
  try:
    _=repr(o)
  except:
    pass
--
Counter([type(att(o)["_object"]) for o in O])

Which gives:

sage: list(Counter([type(att(o)["_object"]) for o in O]))
[<class 'sage.rings.semirings.non_negative_integer_semiring.NonNegativeIntegerSemiring_with_category'>,
 <type 'sage.misc.inherit_comparison.InheritComparisonMetaclass'>,
 <type 'sage.misc.lazy_import.LazyImport'>,
 <class 'sage.combinat.cluster_algebra_quiver.quiver_mutation_type.QuiverMutationTypeFactory'>,
 <type 'module'>,
 <class 'sage.modular.arithgroup.congroup_sl2z.SL2Z_class_with_category'>,
 <class 'sage.categories.cartesian_product.CartesianProductFunctor'>,
 <type 'dict'>,
 <class 'sage.combinat.finite_state_machine_generators.AutomatonGenerators'>,
 <class 'sage.geometry.hyperplane_arrangement.library.HyperplaneArrangementLibrary'>,
 <type 'instance'>,
 <class 'sage.combinat.finite_state_machine_generators.TransducerGenerators'>,
 <class 'sage.libs.gap.libgap.Gap'>,
 <type 'bool'>,
 <type 'str'>,
 <class 'sage.dev.sagedev_wrapper.SageDevWrapper'>,
 <type 'NoneType'>,
 <type 'sage.misc.classcall_metaclass.ClasscallMetaclass'>,
 <class 'sage.interfaces.genus2reduction.Genus2reduction'>,
 <type 'function'>,
 <class 'sage.interfaces.maxima_lib.MaximaLib'>,
 <type 'builtin_function_or_method'>,
 <type 'list'>,
 <type 'type'>,
 <class 'sage.databases.findstat.FindStat'>,
 <class 'sage.rings.invariant_theory.InvariantTheoryFactory'>,
 <class 'sage.misc.inherit_comparison.InheritComparisonClasscallMetaclass'>,
 <class 'abc.ABCMeta'>]

Most of these objects are straightforward callables, that are unlikely to be used as objects to be worked with. The remaining objects could be investigated if they need to by lazily imported by themselves or whether some surrounding scope can be lazily imported instead.

comment:25 in reply to: ↑ 24 Changed 6 years ago by jdemeyer

Replying to nbruin:

Replying to jdemeyer:

Now the question becomes: to what extent does equality matter? One of my proposals is to fix lazy imports in cached functions. That would seriously reduce the extent to which equality matters. It would certainly fix this:

sage: from sage.misc.lazy_import import LazyImport
sage: lazyZZ = LazyImport('sage.rings.integer_ring', 'ZZ')
sage: PolynomialRing(lazyZZ, 'x') is PolynomialRing(ZZ, 'x')
False

What do you think about this?

I know that Robert Bradshaw did not endorse the use of LazyImport for imports like this.

Sure, it was just to give some kind of "worst case" example, with no namespace binding at all.

comment:26 in reply to: ↑ 24 ; follow-up: Changed 6 years ago by jdemeyer

Replying to nbruin:

You can of course try to extend the use of lazy_import and see if you can get it to work for things like NN as well.

I would certainly like to try that. It would be a pity if rings (like NN) are fundamentally incompatible with lazy_import. Consider arb for example: it makes sense to define a global RBF (for real ball field), analogous to RR. It also makes a lot of sense to lazily import this: it's unlikely to be commonly used, it would save an import of at least 2 Python modules and it would save the loading of the external library libarb.

comment:27 in reply to: ↑ 26 ; follow-up: Changed 6 years ago by nbruin

Replying to jdemeyer:

Replying to nbruin:

You can of course try to extend the use of lazy_import and see if you can get it to work for things like NN as well.

I would certainly like to try that. It would be a pity if rings (like NN) are fundamentally incompatible with lazy_import. Consider arb for example: it makes sense to define a global RBF (for real ball field), analogous to RR. It also makes a lot of sense to lazily import this: it's unlikely to be commonly used, it would save an import of at least 2 Python modules and it would save the loading of the external library libarb.

We can do that at the cost of a namespace:

realballfield.RBF
natural_numbers.NN

(where realballfield and natural_numbers can be lazily imported) or at the cost of an accessor function (lazily imported):

RealBallField()
NaturalNumbers()

The fact that

D=dict()
D[NN]=1
D[NN]=2

will never work properly makes trying to pretend otherwise a non-starter in my opinion. You'd have to reach too deeply into python (you probably wouldn't even be able to distinguish between the "initialization" code where the Lazy object needs to be passed and genuine access, where you do need to dereference).

We could reach into various sage infrastructures and special-case LazyImports? there, but in my opinion that would just increase the surprise factor when other constructs fail.

comment:28 in reply to: ↑ 27 ; follow-up: Changed 6 years ago by jdemeyer

Replying to nbruin:

You'd have to reach too deeply into python

Are you thinking of ma_lookup()? That could work, but I agree that it would be a major hack.

comment:29 in reply to: ↑ 28 ; follow-up: Changed 6 years ago by nbruin

Replying to jdemeyer:

Are you thinking of ma_lookup()? That could work, but I agree that it would be a major hack.

That would be one hook. But there's more.

sage: lazy_import("sage.rings.integer_ring","ZZ")
sage: id(ZZ)
140457737914168
sage: ZZ
Integer Ring
sage: id(ZZ)
140458433423744

Note that the user didn't rebind ZZ and yet its identity has changed. That can affect all kinds of things. You'd basically need LazyImport to be transparent for being put into argument lists as well. And that's just horrible (and undebuggable!). I think it would be a major maintainability penalty to try and support that kind of stuff, for relatively little gain. I would say the minor inconvenience for users that objects supported by code that is lazily imported cannot be available straight away in the top namespace is preferable.

But perhaps your experiments end up indicating something else.

comment:30 in reply to: ↑ 29 ; follow-up: Changed 6 years ago by jdemeyer

Replying to nbruin:

I would say the minor inconvenience for users that objects supported by code that is lazily imported cannot be available straight away in the top namespace is preferable.

So, what's your proposal then? Close this ticket as wontfix and change the lazy_import of NN (and similar objects) to a "real" import?

But perhaps your experiments end up indicating something else.

Which "experiments" do you mean? You mean with ma_lookup()?

comment:31 in reply to: ↑ 30 Changed 6 years ago by nbruin

Replying to jdemeyer:

So, what's your proposal then? Close this ticket as wontfix and change the lazy_import of NN (and similar objects) to a "real" import?

Yes. Or if NN is too expensive to import at startup, adapt the places where NN is accessed (the count above shows about 10 modules (including __main__ and I think some double counting) that reference it via a lazy import) in a way that triggers the import as soon as NN is accessed (i.e., either by accessing NN through a namespace that's lazily imported or via an accessor function). I don't see a reasonable alternative while maintaining a semantics model that at least resembles that of python.

The other thing we should do is clean up the mess of mis-lazy_imported objects. As the list above shows, a lot of lazy_imports don't have a chance of ever clearing themselves up. The penalty of accessing an object through a lazy_import isn't that high, but the problem is these things compound. We already have lazy_imports that point to lazy_imports.

I propose:

  • adding a function that gives access to the lazy_import cdef attributes (probably just by returning a dict that has their values)
  • including a doctest that tests if lazy_import objects in sys.modules dicts reference their own scope.

but perhaps that's better handled on a different ticket.

comment:32 Changed 5 years ago by jdemeyer

  • Branch set to u/jdemeyer/lazy_import_breaks_cachedrepresentation

comment:33 Changed 5 years ago by jdemeyer

  • Commit set to d50160173a59f660a41acefb9375fdf0db98b432
  • Status changed from new to needs_review

New commits:

d501601WithEqualityById.__richcmp__: bail out if "other" is no instance of WithEqualityById

comment:34 Changed 5 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Milestone changed from sage-6.10 to sage-8.0

comment:35 Changed 5 years ago by git

  • Commit changed from d50160173a59f660a41acefb9375fdf0db98b432 to 2f5aeadfddb18d81d690015321cadf1bd3ee1227

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

2f5aeadWithEqualityById.__richcmp__: bail out if "other" is no instance of WithEqualityById

comment:36 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:37 Changed 5 years ago by jdemeyer

  • Dependencies set to #23102

comment:38 Changed 5 years ago by git

  • Commit changed from 2f5aeadfddb18d81d690015321cadf1bd3ee1227 to 759da0624d72b123c6b29fc26d1084ed1cd1059c

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

256bf19Move richcmp stuff to new file richcmp.pyx
b591b78Support __richcmp__ in Python classes
47f97ecImplement RealSet comparisons using __richcmp__
acfd0aaTypo
759da06WithEqualityById.__richcmp__: bail out if "other" is no instance of WithEqualityById

comment:39 Changed 5 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:40 Changed 5 years ago by git

  • Commit changed from 759da0624d72b123c6b29fc26d1084ed1cd1059c to 866eddd0d1982a0327ac37daf29bab857f227fa5

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

49b6f76Support __richcmp__ in Python classes
d343d9dImplement RealSet comparisons using __richcmp__
3aac3ccTypo
86e9fcfInstall slot wrappers for rich comparison methods
4d1021bFurther fixes and tests for richcmp_method
866edddWithEqualityById.__richcmp__: bail out if "other" is no instance of WithEqualityById

comment:41 Changed 4 years ago by embray

  • Status changed from needs_review to positive_review

I haven't read all the comments on this ticket, but the problem statement is clear and the proposed fix makes perfect sense.

comment:42 Changed 4 years ago by embray

  • Reviewers set to Erik Bray

comment:43 Changed 4 years ago by vbraun

  • Branch changed from u/jdemeyer/lazy_import_breaks_cachedrepresentation to 866eddd0d1982a0327ac37daf29bab857f227fa5
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.