Opened 6 years ago

Closed 5 years ago

#15618 closed defect (fixed)

Use the correct categories for coercion and conversion maps

Reported by: pbruin Owned by:
Priority: major Milestone: sage-6.4
Component: coercion Keywords: conversion map
Cc: jpflori, SimonKing, cremona, nthiery Merged in:
Authors: Peter Bruin Reviewers: Jean-Pierre Flori, Nils Bruin
Report Upstream: N/A Work issues:
Branch: 54a6eca (Commits) Commit: 54a6ecae11f902b11ad651c67af021af6ed2da21
Dependencies: #16401, #16402, #17364 Stopgaps:

Description (last modified by pbruin)

The following examples show that some convert_maps are incorrectly assumed to be homomorphisms:

sage: f = ZZ.convert_map_from(QQ)
sage: f.parent()
Set of Homomorphisms from Rational Field to Integer Ring
sage: f = ZZ.convert_map_from(GF(2))
sage: f.parent() 
Set of field embeddings from Finite Field of size 2 to Integer Ring
sage: f = GF(11).convert_map_from(GF(7))
sage: f
Conversion map:
  From: Finite Field of size 7
  To:   Finite Field of size 11
sage: f.parent()
Set of field embeddings from Finite Field of size 7 to Finite Field of size 11

This has the effect of constructing an element of a set that is in fact empty. A conversion map should in general only be a morphisms in the category of sets with partial maps, because it is not necessarily defined everywhere. (In case it is defined everywhere, it may of course be placed in the category of sets.)

This ticket fixes the problem in two steps:

  • use SetsWithPartialMaps as the category for DefaultConvertMap;
  • make internal ("reference-weakened") coercion and conversion maps remember their category (the fact that this didn't work was revealed by the above change).

See also https://groups.google.com/forum/#!topic/sage-devel/Z4iNgVMFoms

Change History (41)

comment:1 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:2 Changed 5 years ago by pbruin

  • Description modified (diff)

comment:3 Changed 5 years ago by pbruin

  • Branch set to u/pbruin/15618-DefaultConvertMap
  • Commit set to e772cb79d115b7ed494fc1ac4eaff9120da37711

In the attached branch, I tried to make DefaultConvertMap return a morphism in the category of schemes with partial maps. Unfortunately, a recent change in #14711 (where certain references in the coercion system were made into weak references) makes the map forget its parent Homset, and hence in which category it lives.

Example (cf. the code producing the debugging output):

sage: ZZ.convert_map_from(CC)
old parent = Set of Morphisms from Complex Field with 53 bits of precision to Integer Ring in Category of sets with partial maps
new parent = Set of Homomorphisms from Complex Field with 53 bits of precision to Integer Ring
Conversion map:
  From: Complex Field with 53 bits of precision
  To:   Integer Ring

comment:4 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:5 Changed 5 years ago by git

  • Commit changed from e772cb79d115b7ed494fc1ac4eaff9120da37711 to d0e43bc8bedb2ea6877001c49abb4a56a6b1e145

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

0e48aa9make DefaultConvertMap a morphism in SetsWithPartialMaps()
d92ec90add Map._category_for and Map._set_parent()
d0e43bcTrac 15618: fix category of the Q_to_Z morphism

comment:6 Changed 5 years ago by pbruin

  • Created changed from 01/02/14 00:26:45 to 01/02/14 00:26:45
  • Modified changed from 05/09/14 10:50:02 to 05/09/14 10:50:02
  • Status changed from new to needs_work

Basically works, needs documentation and there are various failing doctests.

comment:7 Changed 5 years ago by git

  • Commit changed from d0e43bc8bedb2ea6877001c49abb4a56a6b1e145 to d96f19ef8b9b418ed974d130de22f73e7e7979e0

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

d96f19eTrac 15618: do not change category in FiniteDimensionalAlgebra._Hom_()

comment:8 Changed 5 years ago by pbruin

Remaining doctest failures (the first is due to #16311):

sage -t --long src/sage/structure/sage_object.pyx  # 1 doctest failed
sage -t --long src/sage/groups/additive_abelian/additive_abelian_wrapper.py  # 2 doctests failed

comment:9 Changed 5 years ago by pbruin

  • Dependencies set to #16401

This dependency is needed to get the category right for finite field embeddings.

comment:10 Changed 5 years ago by pbruin

  • Dependencies changed from #16401 to #16401, #16402

Needed to get the category right for homesets whose domain is a FGP_Module.

Last edited 5 years ago by pbruin (previous) (diff)

comment:11 Changed 5 years ago by pbruin

After #16401 and #16402, the only remaining problem is an unpickling failure of 3 old pickles:

File "src/sage/structure/sage_object.pyx", line 1346, in sage.structure.sage_object.unpickle_all
Failed example:
    sage.structure.sage_object.unpickle_all()  # (4s on sage.math, 2011)
Expected:
    doctest:... DeprecationWarning: This class is replaced by Matrix_modn_dense_float/Matrix_modn_dense_double.
    See http://trac.sagemath.org/4260 for details.
    Successfully unpickled ... objects.
    Failed to unpickle 0 objects.
Got:
    doctest:1: DeprecationWarning: OrderedAlphabet is deprecated; use Alphabet instead.
    See http://trac.sagemath.org/8920 for details.
    doctest:839: DeprecationWarning: This class is replaced by Matrix_modn_dense_float/Matrix_modn_dense_double.
    See http://trac.sagemath.org/4260 for details.
     * unpickle failure: load('/home/pbruin/.sage/temp/selmer/22153/dir_6q6dfL//pickle_jar/_class__sage_modular_hecke_degenmap_DegeneracyMap__.sobj')
     * unpickle failure: load('/home/pbruin/.sage/temp/selmer/22153/dir_6q6dfL//pickle_jar/_type__sage_rings_morphism_RingHomomorphism_cover__.sobj')
     * unpickle failure: load('/home/pbruin/.sage/temp/selmer/22153/dir_6q6dfL//pickle_jar/_type__sage_rings_morphism_RingHomomorphism_from_quotient__.sobj')
    Failed:
    _class__sage_modular_hecke_degenmap_DegeneracyMap__.sobj
    _type__sage_rings_morphism_RingHomomorphism_cover__.sobj
    _type__sage_rings_morphism_RingHomomorphism_from_quotient__.sobj
    ----------------------------------------------------------------------
    ** This error is probably due to an old pickle failing to unpickle.
    ** See sage.structure.sage_object.register_unpickle_override for
    ** how to override the default unpickling methods for (old) pickles.
    ** NOTE: pickles should never be removed from the pickle_jar!
    ----------------------------------------------------------------------
    Successfully unpickled 583 objects.
    Failed to unpickle 3 objects.

comment:12 Changed 5 years ago by git

  • Commit changed from d96f19ef8b9b418ed974d130de22f73e7e7979e0 to d261a37914aec9a0b7949fd526881003d3214f62

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

d261a37Trac 15618: delay setting _category_for to avoid unpickling trouble

comment:13 Changed 5 years ago by pbruin

  • Authors set to Peter Bruin
  • Description modified (diff)
  • Summary changed from convert_map should not be a map to Use the correct categories for coercion and conversion maps
  • Work issues set to add doctests

The new commit solves the unpickling problem, all tests now pass. (The pickles weren't necessarily broken, my patch was trying to access the category of partially-unpickled homsets.)

comment:14 Changed 5 years ago by git

  • Commit changed from d261a37914aec9a0b7949fd526881003d3214f62 to 54a6ecae11f902b11ad651c67af021af6ed2da21

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

54a6ecaTrac 15618: add doctests

comment:15 Changed 5 years ago by pbruin

  • Description modified (diff)
  • Status changed from needs_work to needs_review
  • Work issues add doctests deleted

Note to reviewer(s): the branches of the dependencies (#16401, #16402) are not merged in this branch.

comment:16 follow-up: Changed 5 years ago by pbruin

In the current branch, there is a new Map._category_for attribute that is always a strong reference. I later thought that instead of this, it might be better to treat category_for in an analogous way to domain, i.e. to make it an attribute that can be either a ConstantFunction or to a weakref. I'm undecided for the moment, so I'm keeping the original branch in the ticket description.

The alternative branch (including #16401 and #16402) is u/pbruin/15618-convert_map_category (diff, commits).

comment:17 Changed 5 years ago by pbruin

  • Branch changed from u/pbruin/15618-DefaultConvertMap to u/pbruin/15618-with-dependencies
  • Commit changed from 54a6ecae11f902b11ad651c67af021af6ed2da21 to 1ae3601bdb7e02d9a554b7c6e837751e1cd62b5a

The dependencies are merged in this branch, for the benefit of reviewers and the patchbot. The current branch is now simply one commit behind the branch u/pbruin/15618-convert_map_category of comment:16.

comment:18 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:19 Changed 5 years ago by pbruin

  • Branch changed from u/pbruin/15618-with-dependencies to u/pbruin/15618-DefaultConvertMap
  • Commit changed from 1ae3601bdb7e02d9a554b7c6e837751e1cd62b5a to 54a6ecae11f902b11ad651c67af021af6ed2da21

(Setting the branch back to u/pbruin/15618-DefaultConvertMap since the dependencies have now been merged into develop and the previous branch would not merge automatically.)

comment:20 Changed 5 years ago by jpflori

  • Status changed from needs_review to needs_work

This looks like an improvement to me.

But it gives me a bunch of failures (on top of 6.5.beta0):

sage -t --long --warn-long 156.2 src/sage/schemes/generic/homset.py  # 1 doctest failed
sage -t --long --warn-long 156.2 src/sage/schemes/generic/scheme.py  # 1 doctest failed
sage -t --long --warn-long 156.2 src/sage/schemes/projective/projective_homset.py  # 1 doctest failed
sage -t --long --warn-long 156.2 src/sage/schemes/generic/spec.py  # 1 doctest failed
sage -t --long --warn-long 156.2 src/sage/schemes/generic/morphism.py  # 19 doctests failed
sage -t --long --warn-long 156.2 src/sage/schemes/affine/affine_homset.py  # 1 doctest failed
sage -t --long --warn-long 156.2 src/sage/categories/schemes.py  # 4 doctests failed

comment:21 Changed 5 years ago by jpflori

All errors seem to be of the following form:

f = S(g)
...
ValueError: Set of morphisms
      From: Spectrum of ...
      To:   Spectrum of ... is not in Category of sets with partial maps

comment:22 Changed 5 years ago by jpflori

I guess #10668 is the culprit.

@Nicolas: any trivial fix for this?

comment:23 Changed 5 years ago by pbruin

The problem seems to be that the category Schemes().Homsets() does not have Sets() as a supercategory:

sage: Sets().Homsets().all_super_categories()
[Category of homsets of sets,
 Category of homsets,
 Category of sets,
 Category of sets with partial maps,
 Category of objects]
sage: Schemes().Homsets().all_super_categories()
[Category of homsets of schemes,
 Category of objects]

comment:24 Changed 5 years ago by jpflori

I kind of think the same thing.

For sure, also getting Nicolas' mind would be helpful :)

comment:25 Changed 5 years ago by git

  • Commit changed from 54a6ecae11f902b11ad651c67af021af6ed2da21 to bb452a7f168890c3dbeaf049c5a4537e5827cb0e

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

02702bdMerge branch 'develop' into ticket/15618-DefaultConvertMap
bb452a7Trac 15618: remove incorrect class Schemes.Homsets

comment:26 follow-ups: Changed 5 years ago by pbruin

  • Cc nthiery added

The last commit seems to fix the bugs.

Nicolas: I think we can safely remove the Schemes.Homsets class with its extra_super_categories method, as in this commit. However, is it the intended behaviour that the former presence of this class + method caused Schemes().Homsets() to acquire fewer supercategories? What is the explanation for this?

comment:27 Changed 5 years ago by git

  • Commit changed from bb452a7f168890c3dbeaf049c5a4537e5827cb0e to b3cc973ebd7c453847e71031c63f10715866fe2c

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

b3cc973Trac 15618: fix typesetting

comment:28 Changed 5 years ago by pbruin

  • Status changed from needs_work to needs_review

comment:29 Changed 5 years ago by git

  • Commit changed from b3cc973ebd7c453847e71031c63f10715866fe2c to 54a6ecae11f902b11ad651c67af021af6ed2da21

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

comment:30 Changed 5 years ago by pbruin

  • Dependencies changed from #16401, #16402 to #16401, #16402, #17359

I think it is better to solve that problem on a new ticket. This is now #17359.

comment:31 in reply to: ↑ 16 Changed 5 years ago by pbruin

Replying to pbruin:

In the current branch, there is a new Map._category_for attribute that is always a strong reference. I later thought that instead of this, it might be better to treat category_for in an analogous way to domain, i.e. to make it an attribute that can be either a ConstantFunction or to a weakref. I'm undecided for the moment, so I'm keeping the original branch in the ticket description.

Since the alternative branch u/pbruin/15618-convert_map_category may still be relevant (to prevent potential memory leaks), I rebased it (diff, commits).

comment:32 follow-ups: Changed 5 years ago by nbruin

Just this line:

if category.is_subcategory(FiniteDimensionalAlgebrasWithBasis(self.base_ring()))

is a memory leak:

sage: import gc
sage: from collections import Counter
sage: gc.collect()
93
sage: pre={id(a) for a in gc.get_objects()}
sage: 
sage: for p in prime_range(20000):
....:         A =  FiniteDimensionalAlgebrasWithBasis(GF(p))
....:     
sage: gc.collect()
0
sage: post=Counter(str(type(a)) for a in gc.get_objects() if id(a) not in pre)
sage: post
Counter({"<type 'tuple'>": 314633, "<type 'dict'>": 106360, "<type 'list'>": 88464, "<type 'cell'>": 67894, "<type 'sage.misc.cachefunc.CachedMethodCallerNoArgs'>": 52032, "<type 'weakref'>": 40733, "<class 'weakref.KeyedRef'>": 36193, "<type 'function'>": 33947, "<type 'staticmethod'>": 33947, "<type 'builtin_function_or_method'>": 33938, "<type 'sage.misc.cachefunc.CachedMethodCaller'>": 33937, "<type 'frozenset'>": 33933, "<class 'sage.structure.dynamic_class.DynamicClasscallMetaclass'>": 33932, "<type 'sage.rings.finite_rings.integer_mod.IntegerMod_int'>": 25868, "<type 'sage.structure.coerce_dict.MonoDict'>": 4522, "<type 'sage.structure.coerce_dict.MonoDictEraser'>": 4522, "<class 'sage.categories.unital_algebras.UnitalAlgebras_with_category'>": 2262, "<class 'sage.categories.vector_spaces.VectorSpaces.WithBasis_with_category'>": 2262, "<class 'sage.categories.bimodules.Bimodules_with_category'>": 2262, "<class 'sage.categories.unital_algebras.UnitalAlgebras.WithBasis_with_category'>": 2262, "<class 'sage.categories.algebras.Algebras_with_category'>": 2262, "<class 'sage.categories.left_modules.LeftModules_with_category'>": 2262, "<class 'sage.categories.vector_spaces.VectorSpaces_with_category'>": 2262, "<class 'sage.categories.right_modules.RightModules_with_category'>": 2262, "<class 'sage.categories.finite_dimensional_algebras_with_basis.FiniteDimensionalAlgebrasWithBasis_with_category'>": 2262, "<class 'sage.categories.modules_with_basis.ModulesWithBasis_with_category'>": 2262, "<class 'sage.categories.algebras_with_basis.AlgebrasWithBasis_with_category'>": 2262, "<class 'sage.categories.magmatic_algebras.MagmaticAlgebras.WithBasis_with_category'>": 2262, "<class 'sage.categories.associative_algebras.AssociativeAlgebras_with_category'>": 2262, "<class 'sage.categories.modules.Modules_with_category'>": 2262, "<type 'sage.structure.coerce_dict.TripleDict'>": 2261, "<class 'sage.rings.ideal.Ideal_pid'>": 2261, "<type 'sage.rings.finite_rings.integer_mod.NativeIntStruct'>": 2261, "<class 'sage.categories.magmatic_algebras.MagmaticAlgebras_with_category'>": 2261, "<class 'sage.rings.finite_rings.finite_field_prime_modn.FiniteField_prime_modn_with_category'>": 2261, "<type 'sage.structure.coerce_dict.TripleDictEraser'>": 2261, "<type 'instancemethod'>": 2261, "<class 'sage.structure.dynamic_class.DynamicMetaclass'>": 15, "<class '_ast.Call'>": 3, "<type 'sage.misc.constant_function.ConstantFunction'>": 2, "<class '_ast.Name'>": 2, "<type 'frame'>": 1, "<type 'set'>": 1, "<class '_ast.comprehension'>": 1, "<class '_ast.Interactive'>": 1, "<class 'sage.categories.finite_dimensional_modules_with_basis.FiniteDimensionalModulesWithBasis_with_category'>": 1, "<class '_ast.Module'>": 1, "<type 'sage.categories.category_singleton.Category_contains_method_by_parent_class'>": 1, "<type 'listiterator'>": 1, "<type 'generator'>": 1, "<class '_ast.Assign'>": 1, "<class 'sage.categories.modules.Modules.FiniteDimensional_with_category'>": 1})

The problem is that categories themselves have infinite life, so one should not instantiate parametrized categories unless one really has to. To this end we now have:

sage: GF(3)['x'].category()
Join of Category of euclidean domains and Category of commutative algebras over (finite fields and subquotients of monoids and quotients of semigroups)

(as you can see, the category doesn't explicitly link to GF(3) anymore) Admittedly:

sage: FiniteDimensionalAlgebra(GF(3), [Matrix([[1, 0], [0, 1]]), Matrix([[0, 1], [0, 0]])]).category()
Category of finite dimensional algebras with basis over Finite Field of size 3

so the memory leak already exists for FiniteDimensionalAlgebra in general. However, since you're introducing the line here, you should probably argue why you're not making the problem worse and/or open a ticket about the memory leak in general.

comment:33 in reply to: ↑ 26 Changed 5 years ago by jpflori

Replying to pbruin:

The last commit seems to fix the bugs.

Nicolas: I think we can safely remove the Schemes.Homsets class with its extra_super_categories method, as in this commit. However, is it the intended behaviour that the former presence of this class + method caused Schemes().Homsets() to acquire fewer supercategories? What is the explanation for this?

Thanks for cc'ing Nicolas as I forgot to do so!

comment:34 in reply to: ↑ 32 ; follow-up: Changed 5 years ago by jpflori

Replying to nbruin:

Just this line:

if category.is_subcategory(FiniteDimensionalAlgebrasWithBasis(self.base_ring()))

is a memory leak:

sage: import gc
sage: from collections import Counter
sage: gc.collect()
93
sage: pre={id(a) for a in gc.get_objects()}
sage: 
sage: for p in prime_range(20000):
....:         A =  FiniteDimensionalAlgebrasWithBasis(GF(p))
....:     
sage: gc.collect()
0
sage: post=Counter(str(type(a)) for a in gc.get_objects() if id(a) not in pre)
sage: post
Counter({"<type 'tuple'>": 314633, "<type 'dict'>": 106360, "<type 'list'>": 88464, "<type 'cell'>": 67894, "<type 'sage.misc.cachefunc.CachedMethodCallerNoArgs'>": 52032, "<type 'weakref'>": 40733, "<class 'weakref.KeyedRef'>": 36193, "<type 'function'>": 33947, "<type 'staticmethod'>": 33947, "<type 'builtin_function_or_method'>": 33938, "<type 'sage.misc.cachefunc.CachedMethodCaller'>": 33937, "<type 'frozenset'>": 33933, "<class 'sage.structure.dynamic_class.DynamicClasscallMetaclass'>": 33932, "<type 'sage.rings.finite_rings.integer_mod.IntegerMod_int'>": 25868, "<type 'sage.structure.coerce_dict.MonoDict'>": 4522, "<type 'sage.structure.coerce_dict.MonoDictEraser'>": 4522, "<class 'sage.categories.unital_algebras.UnitalAlgebras_with_category'>": 2262, "<class 'sage.categories.vector_spaces.VectorSpaces.WithBasis_with_category'>": 2262, "<class 'sage.categories.bimodules.Bimodules_with_category'>": 2262, "<class 'sage.categories.unital_algebras.UnitalAlgebras.WithBasis_with_category'>": 2262, "<class 'sage.categories.algebras.Algebras_with_category'>": 2262, "<class 'sage.categories.left_modules.LeftModules_with_category'>": 2262, "<class 'sage.categories.vector_spaces.VectorSpaces_with_category'>": 2262, "<class 'sage.categories.right_modules.RightModules_with_category'>": 2262, "<class 'sage.categories.finite_dimensional_algebras_with_basis.FiniteDimensionalAlgebrasWithBasis_with_category'>": 2262, "<class 'sage.categories.modules_with_basis.ModulesWithBasis_with_category'>": 2262, "<class 'sage.categories.algebras_with_basis.AlgebrasWithBasis_with_category'>": 2262, "<class 'sage.categories.magmatic_algebras.MagmaticAlgebras.WithBasis_with_category'>": 2262, "<class 'sage.categories.associative_algebras.AssociativeAlgebras_with_category'>": 2262, "<class 'sage.categories.modules.Modules_with_category'>": 2262, "<type 'sage.structure.coerce_dict.TripleDict'>": 2261, "<class 'sage.rings.ideal.Ideal_pid'>": 2261, "<type 'sage.rings.finite_rings.integer_mod.NativeIntStruct'>": 2261, "<class 'sage.categories.magmatic_algebras.MagmaticAlgebras_with_category'>": 2261, "<class 'sage.rings.finite_rings.finite_field_prime_modn.FiniteField_prime_modn_with_category'>": 2261, "<type 'sage.structure.coerce_dict.TripleDictEraser'>": 2261, "<type 'instancemethod'>": 2261, "<class 'sage.structure.dynamic_class.DynamicMetaclass'>": 15, "<class '_ast.Call'>": 3, "<type 'sage.misc.constant_function.ConstantFunction'>": 2, "<class '_ast.Name'>": 2, "<type 'frame'>": 1, "<type 'set'>": 1, "<class '_ast.comprehension'>": 1, "<class '_ast.Interactive'>": 1, "<class 'sage.categories.finite_dimensional_modules_with_basis.FiniteDimensionalModulesWithBasis_with_category'>": 1, "<class '_ast.Module'>": 1, "<type 'sage.categories.category_singleton.Category_contains_method_by_parent_class'>": 1, "<type 'listiterator'>": 1, "<type 'generator'>": 1, "<class '_ast.Assign'>": 1, "<class 'sage.categories.modules.Modules.FiniteDimensional_with_category'>": 1})

The problem is that categories themselves have infinite life, so one should not instantiate parametrized categories unless one really has to. To this end we now have:

sage: GF(3)['x'].category()
Join of Category of euclidean domains and Category of commutative algebras over (finite fields and subquotients of monoids and quotients of semigroups)

(as you can see, the category doesn't explicitly link to GF(3) anymore) Admittedly:

sage: FiniteDimensionalAlgebra(GF(3), [Matrix([[1, 0], [0, 1]]), Matrix([[0, 1], [0, 0]])]).category()
Category of finite dimensional algebras with basis over Finite Field of size 3

so the memory leak already exists for FiniteDimensionalAlgebra in general. However, since you're introducing the line here, you should probably argue why you're not making the problem worse and/or open a ticket about the memory leak in general.

For clarity, I would suggest to open another ticket for that. And make it a dependency here (or not if we consider the situation is not worsened by the current ticket.)

comment:35 Changed 5 years ago by jpflori

Unless it's solved by Peter's alternative branch of course.

comment:36 in reply to: ↑ 34 Changed 5 years ago by nbruin

Replying to jpflori:

For clarity, I would suggest to open another ticket for that. And make it a dependency here (or not if we consider the situation is not worsened by the current ticket.)

Perhaps use #17360 for that. Please do glance through the rest of your code to check if similar changes are warranted (which can then be implemented either here, on #17360, or another follow-on ticket)

comment:37 in reply to: ↑ 26 Changed 5 years ago by nthiery

Replying to pbruin:

Nicolas: I think we can safely remove the Schemes.Homsets class with its extra_super_categories method, as in this commit.

Cool.

However, is it the intended behaviour that the former presence of this class + method caused Schemes().Homsets() to acquire fewer supercategories? What is the explanation for this?

Thanks for spotting this as this was a bug. See #17364.

comment:38 Changed 5 years ago by pbruin

  • Dependencies changed from #16401, #16402, #17359 to #16401, #16402, #17364

comment:39 in reply to: ↑ 32 Changed 5 years ago by pbruin

Replying to nbruin:

Just this line:

if category.is_subcategory(FiniteDimensionalAlgebrasWithBasis(self.base_ring()))

is a memory leak:

[...]

Admittedly:

sage: FiniteDimensionalAlgebra(GF(3), [Matrix([[1, 0], [0, 1]]), Matrix([[0, 1], [0, 0]])]).category()
Category of finite dimensional algebras with basis over Finite Field of size 3

so the memory leak already exists for FiniteDimensionalAlgebra in general. However, since you're introducing the line here, you should probably argue why you're not making the problem worse and/or open a ticket about the memory leak in general.

This change is necessary because otherwise we get errors if the provided category (which is SetsWithPartialMaps() in some cases) is not a subcategory of FiniteDimensionalAlgebrasWithBasis(self.base_ring()). Logically, the choice whether the homset should be of type FiniteDimensionalAlgebraHomset should depend on the category, not vice versa.

This ticket does not make the memory leak (much) worse, because in most cases B will be a FiniteDimensionalAlgebra anyway, in which case the same category is constructed.

I also tested (using your example and in addition constructing A.coerce_map_from(GF(p)) for each A in the loop) that the current branch does not cause additional memory leaks, and that the existing leak is fixed by #17360 both before and after applying my branch. Thus the alternative branch (comment:31) doesn't seem to be needed.

comment:40 Changed 5 years ago by jpflori

  • Reviewers set to Jean-Pierre Flori, Nils Bruin
  • Status changed from needs_review to positive_review

I agree this does not make the leak much worse. And the leak should be fixed in #17360.

comment:41 Changed 5 years ago by vbraun

  • Branch changed from u/pbruin/15618-DefaultConvertMap to 54a6ecae11f902b11ad651c67af021af6ed2da21
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.