Opened 11 years ago

Closed 7 years ago

#10513 closed defect (fixed)

Coercion and category framework for modules

Reported by: SimonKing Owned by: robertwb
Priority: major Milestone: sage-6.6
Component: coercion Keywords: coercion, category framework, modules
Cc: vbraun, novoselt, jpflori Merged in:
Authors: Simon King, Peter Bruin Reviewers: Jean-Pierre Flori, Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: 9865576 (Commits, GitHub, GitLab) Commit: 9865576ec5600a1502e5c5e455e5f2ce08406f4c
Dependencies: #16507, #17578, #17561, #18040, #17562 Stopgaps:

Status badges

Description (last modified by pbruin)

Currently, modules in Sage are based on the old coercion model and do not cooperate well with the category framework. This ticket updates almost all module classes to the use new coercion model and the category framework.

Moreover, some coercions go wrong:

sage: V3 = QQ^3
sage: M = V3 / [[1,2,3]]
sage: V2 = QQ^2
sage: N = V3 / [[1,1,1]]
# The quotient maps are of course coercions
sage: f3M = M.coerce_map_from(V3)
sage: f3N = N.coerce_map_from(V3)
# The following is a bug...
sage: V2.has_coerce_map_from(N)
True
sage: M.has_coerce_map_from(V2)
True
sage: N.has_coerce_map_from(V2)
True
#... because the composition of coercion
# from N to V2 to M should be a coercion,
# but it isn't:
sage: M.has_coerce_map_from(N)
False
# Moreover, we obtain a non-commuting triangle of
# coercions:
sage: fM2 = V2.coerce_map_from(M)
sage: fN2 = V2.coerce_map_from(N)
sage: [fM2(f3M(x)) for x in V3.basis()]
[(1, 0), (0, 1), (-1/3, -2/3)]
sage: [fN2(f3N(x)) for x in V3.basis()]
[(1, 0), (0, 1), (-1, -1)]

Comparison goes wrong as well:

sage: V2==M
True
sage: M==V2
False

In the above examples, this ticket removes the coercion between abstract module V^2 and the quotient modules, and makes sure that V2!=M.

Attachments (1)

trac10513-coercion_and_categories_for_modules.patch (29.7 KB) - added by SimonKing 11 years ago.
Implement coercion and category framework for free modules, submodules, quotient modules. Depends on #10496

Download all attachments as: .zip

Change History (69)

Changed 11 years ago by SimonKing

Implement coercion and category framework for free modules, submodules, quotient modules. Depends on #10496

comment:1 Changed 11 years ago by SimonKing

  • Status changed from new to needs_review
  • Type changed from PLEASE CHANGE to defect

For the patchbot/release manager:

depends on #10496

Ready for review!

comment:2 follow-up: Changed 11 years ago by vbraun

  • Status changed from needs_review to needs_info

One of the things that I did in #9713 was to replace sage.structure.parent_gens.ParentWithAdditiveAbelianGens with sage.structure.parent.Parent as the base class of Module. At the top of sage/structure/parent_gens.pyx it says that

   This class is being deprecated, see
   ``sage.structure.parent.Parent`` and
   ``sage.structure.category_object.CategoryObject`` for the new
   model.

But your patch does not change the Module base class. So I'm a bit confused about what the right way of doing things is.

comment:3 in reply to: ↑ 2 Changed 11 years ago by SimonKing

Replying to vbraun:

At the top of sage/structure/parent_gens.pyx it says that

   This class is being deprecated, see
   ``sage.structure.parent.Parent`` and
   ``sage.structure.category_object.CategoryObject`` for the new
   model.

But your patch does not change the Module base class.

Thank you, I simply wasn't aware of the deprecation. Changing it now, and if tests pass I'll update my patch.

comment:4 Changed 11 years ago by SimonKing

  • Status changed from needs_info to needs_work

I just noticed that ParentWithBase is requested in various other files. So, it will take a while until I can replace ParentWithAdditiveAbelianGens by Parent.

comment:5 Changed 11 years ago by SimonKing

  • Work issues set to replace ParentWithAdditiveGens by Parent; arithmetic for module elements using single underscore methods

comment:6 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:7 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:8 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:9 Changed 7 years ago by pbruin

  • Authors changed from Simon King to Simon King, Peter Bruin
  • Dependencies set to #16507

I am trying to finish this ticket. I currently have a branch that passes all doctests, but I am still improving some of the code and adding documentation.

comment:10 follow-up: Changed 7 years ago by pbruin

  • Branch set to u/pbruin/10513-coercion_and_categories_for_modules
  • Commit set to ed13085009f4b79544e1aa9970820b7f1bf6cb8f
  • Status changed from needs_work to needs_review
  • Work issues replace ParentWithAdditiveGens by Parent; arithmetic for module elements using single underscore methods deleted

OK, ready for review. There is still some future work to be done:

  • move the last few classes from Module_old to Module. There are only four of these remaining:
    1. sage.coding.linear_code.LinearCode
    2. sage.modular.abvar.finite_subgroup.FiniteSubgroup
    3. sage.modular.overconvergent.genus0.OverconvergentModularFormsSpace
    4. sage.structure.formal_sum.FormalSums
  • make sage.modules.matrix_morphism.MatrixMorphism_abstract use single-underscore methods for addition, subtraction, scalar multiplication and composition.

I think it is better to do these in separate tickets.

comment:11 Changed 7 years ago by tscrim

sage.coding.linear_code.LinearCode is taken care of in #16644.

comment:12 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:13 Changed 7 years ago by git

  • Commit changed from ed13085009f4b79544e1aa9970820b7f1bf6cb8f to 5126b66a6c5ba2d52b6daff6ee489aa8b48a7548

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

5126b66Trac 10513: remove source line number in doctest

comment:14 Changed 7 years ago by pbruin

  • Description modified (diff)

comment:15 Changed 7 years ago by git

  • Commit changed from 5126b66a6c5ba2d52b6daff6ee489aa8b48a7548 to ddda401f855fc34936e204eb14fa3d0af597b76f

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

ddda401Merge branch 'develop' into ticket/10513-coercion_and_categories_for_modules

comment:16 Changed 7 years ago by jpflori

  • Cc jpflori added

comment:17 Changed 7 years ago by git

  • Commit changed from ddda401f855fc34936e204eb14fa3d0af597b76f to ed6bbf1bf478d40fe467d2f56bc58b0ad482a840

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

2bb997bMerge branch 'develop' into ticket/10513-coercion_and_categories_for_modules
ed6bbf1Trac 10513: fix doctest

comment:18 Changed 7 years ago by jpflori

Could you add a little documentation about the coercion_reverse thing? Or rather, move it in the actual code, rather than in comments? Not that the category/coercion stuff is already obscure, but...

comment:19 Changed 7 years ago by jpflori

I guess my comment should have been made in #16507.

comment:20 Changed 7 years ago by pbruin

  • Status changed from needs_review to needs_work
  • Work issues set to fix pickling problem

After merging with 6.5.beta3 (which git does automatically, despite Trac being unable to auto-merge), there is one failing doctest:

File "src/sage/modules/free_module.py", line 6074, in sage.modules.free_module.FreeModule_submodule_with_basis_field
Failed example:
    TestSuite(W).run()
Expected nothing
Got:
    Failure in _test_pickling:
    Traceback (most recent call last):
      File "/home/bruinpj/src/sage/local/lib/python2.7/site-packages/sage/misc/sage_unittest.py", line 282, in run
        test_method(tester = tester)
      File "sage/structure/sage_object.pyx", line 543, in sage.structure.sage_object.SageObject._test_pickling (build/cythonized/sage/structure/sage_object.c:4553)
        tester.assertEqual(loads(dumps(self)), self)
      File "/home/bruinpj/src/sage/local/lib/python/unittest/case.py", line 513, in assertEqual
        assertion_func(first, second, msg=msg)
      File "/home/bruinpj/src/sage/local/lib/python/unittest/case.py", line 506, in _baseAssertEqual
        raise self.failureException(msg)
    AssertionError: Vector space of degree 3 and dimension 1 over Fraction Field of Univariate Polynomial Ring in x over Rational Field
    User basis matrix:
    [1 1 x] != Vector space of degree 3 and dimension 1 over Fraction Field of Univariate Polynomial Ring in x over Rational Field
    User basis matrix:
    [1 1 x]
    ------------------------------------------------------------
    The following tests failed: _test_pickling

This is due to the cached echelonized_basis_matrix behaving badly under pickling:

sage: K.<x> = FractionField(QQ['x'])
sage: M = K^3
sage: W = M.span_of_basis([[1, 1, x]])
sage: W.echelonized_basis_matrix()
[1 1 x]
sage: s = dumps(W)
sage: V = loads(s)
sage: V.echelonized_basis_matrix()
[1, 1, x]
sage: type(_)
<type 'list'>

There does not seem to be any such problem with basis_matrix():

sage: W.basis_matrix()
[1 1 x]
sage: V.basis_matrix()
[1 1 x]

comment:21 Changed 7 years ago by pbruin

  • Dependencies changed from #16507 to #16507, #17527
  • Work issues fix pickling problem deleted

The pickling problem is independent of this ticket, see #17527.

comment:22 Changed 7 years ago by git

  • Commit changed from ed6bbf1bf478d40fe467d2f56bc58b0ad482a840 to 2f52a3cb9945479d4d7298c06119bde5481c6d27

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

a19e2c5Merge branch 'develop' into ticket/10513-coercion_and_categories_for_modules
2f52a3cTrac 10513: remove unnecessary FreeModule_generic_field.__echelonized_basis_matrix

comment:23 Changed 7 years ago by pbruin

  • Dependencies changed from #16507, #17527 to #16507
  • Status changed from needs_work to needs_review

The pickling problem can actually be circumvented in a more elegant way by avoiding an unnecessary caching step (the echelon form of a matrix is already cached).

comment:24 Changed 7 years ago by git

  • Commit changed from 2f52a3cb9945479d4d7298c06119bde5481c6d27 to 2d7f0759c12fbeeb45d42c2d6160f8d925a22252

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

2d7f075Trac 10513: small documentation improvement

comment:25 in reply to: ↑ 10 ; follow-up: Changed 7 years ago by jpflori

I had a look at the work done here and it looks nice.

Replying to pbruin:

OK, ready for review. There is still some future work to be done:

  • move the last few classes from Module_old to Module. There are only four of these remaining:
    1. sage.coding.linear_code.LinearCode
    2. sage.modular.abvar.finite_subgroup.FiniteSubgroup
    3. sage.modular.overconvergent.genus0.OverconvergentModularFormsSpace
    4. sage.structure.formal_sum.FormalSums
  • make sage.modules.matrix_morphism.MatrixMorphism_abstract use single-underscore methods for addition, subtraction, scalar multiplication and composition.

I think it is better to do these in separate tickets.

Did you open tickets for these?

And in a couple of changes to src/sage/geometry/cone.py you modify code not to use _ in _ as ther is no coercion defined. Any ticket opened to implement such a coercion?

comment:26 in reply to: ↑ 25 ; follow-up: Changed 7 years ago by pbruin

Replying to jpflori:

I had a look at the work done here and it looks nice.

Thanks!

Replying to pbruin:

OK, ready for review. There is still some future work to be done:

  • move the last few classes from Module_old to Module. There are only four of these remaining:
    1. sage.coding.linear_code.LinearCode
    2. sage.modular.abvar.finite_subgroup.FiniteSubgroup
    3. sage.modular.overconvergent.genus0.OverconvergentModularFormsSpace
    4. sage.structure.formal_sum.FormalSums
  • make sage.modules.matrix_morphism.MatrixMorphism_abstract use single-underscore methods for addition, subtraction, scalar multiplication and composition.

I think it is better to do these in separate tickets.

Did you open tickets for these?

I just created #17543 and #17544. (See comment:11 for LinearCode.)

And in a couple of changes to src/sage/geometry/cone.py you modify code not to use _ in _ as ther is no coercion defined. Any ticket opened to implement such a coercion?

I'm not sure if this is desired; the absence of these particular maps may have been an (unintended) consequence of implementing the new coercion model in the simplest possible way, but it may have been intentional as well, I don't remember.

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

Replying to pbruin:> > Any ticket opened to implement such a coercion?

I'm not sure if this is desired; the absence of these particular maps may have been an (unintended) consequence of implementing the new coercion model in the simplest possible way, but it may have been intentional as well, I don't remember.

I was asking because it used to be possible to use the in keyword when the classes were using the old coercion model.

Last edited 7 years ago by jpflori (previous) (diff)

comment:28 in reply to: ↑ 27 Changed 7 years ago by pbruin

Replying to jpflori:

Replying to pbruin:> > Any ticket opened to implement such a coercion?

I'm not sure if this is desired; the absence of these particular maps may have been an (unintended) consequence of implementing the new coercion model in the simplest possible way, but it may have been intentional as well, I don't remember.

I was asking because it used to be possible to use the in keyword when the classes were using the old coercion model.

After looking at this again, I think the reason was as follows: a cone C is a certain type of subset of a vector space V, and C contains a largest maximal linear subspace S. Now to test if an element x of C is in S, the coercion framework checks if S(x) == x, but when trying to comparing these two elements it cannot find a common parent for the toric lattice and the linear subspace. This seems to be because of the following method in ToricLattice_generic:

# We need to override this function, otherwise e.g. the sum of elements of
# different lattices of the same dimension will live in ZZ^n.
def construction(self):
    r"""
    Return the functorial construction of ``self``.

    OUTPUT:

    - ``None``, we do not think of toric lattices as constructed from
      simpler objects since we do not want to perform arithmetic involving
      different lattices.

    [...]
    """
    return None

The reason why it used to work with the old coercion model might have been a peculiarity of some _coerce_impl() method. Trying to fix this (so that we can use ... in ... again) will probably lead to all kind of other problems, so I'd prefer not to do that on this ticket.

comment:29 Changed 7 years ago by jpflori

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

Ok, I buy your explanation, thanks for digging this out.

comment:30 follow-up: Changed 7 years ago by vbraun

I get lots of doctest failures all originating at something like

    TypeError: must be intialized with a type, not Manin Symbol List of weight 4 for Gamma0(11)

comment:31 Changed 7 years ago by vbraun

  • Status changed from positive_review to needs_work

comment:32 in reply to: ↑ 30 Changed 7 years ago by pbruin

Replying to vbraun:

I get lots of doctest failures all originating at something like

    TypeError: must be intialized with a type, not Manin Symbol List of weight 4 for Gamma0(11)

Can you give more details and/or a link to a log file? I am not getting any doctest failures in sage/modular (which I would guess is where these failures occur) after merging this branch with 6.5.beta4, both on x86_64 and on ARM.

comment:33 follow-up: Changed 7 years ago by jdemeyer

I think it's because of #10962.

comment:34 follow-up: Changed 7 years ago by jdemeyer

Does this patch address anything of https://groups.google.com/forum/#!topic/sage-devel/7n5xtT9Dw2g by chance?

comment:35 in reply to: ↑ 34 Changed 7 years ago by pbruin

Replying to jdemeyer:

Does this patch address anything of https://groups.google.com/forum/#!topic/sage-devel/7n5xtT9Dw2g by chance?

No, I don't think so. (It does solve #17576, though.)

comment:36 in reply to: ↑ 33 Changed 7 years ago by pbruin

Replying to jdemeyer:

I think it's because of #10962.

You're right, I'm looking into it.

comment:37 Changed 7 years ago by pbruin

  • Dependencies changed from #16507 to #16507, #17578
  • Status changed from needs_work to positive_review

The failures when applying this together with #10962 are caused by the fact that Manin symbols do not use the Parent/Element machinery. This is fixed by #17578.

comment:38 follow-up: Changed 7 years ago by jdemeyer

  • Status changed from positive_review to needs_info

In src/sage/modules/free_module_element.pyx, you changed in one place

except TypeError:

to

except (TypeError, ValueError):

Is this change needed? If yes, it should also be applied to the other place with similar code (there is one __init__ method for dense and one for sparse vectors).

comment:39 Changed 7 years ago by jdemeyer

(I'm asking because this is a conflict with #17561)

comment:40 Changed 7 years ago by git

  • Commit changed from 2d7f0759c12fbeeb45d42c2d6160f8d925a22252 to 99ec4cc13099658e0bc762a6e8d230c3956c7150

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

99ec4ccTrac 10513: revert a change in try...except

comment:41 in reply to: ↑ 38 Changed 7 years ago by pbruin

Replying to jdemeyer:

In src/sage/modules/free_module_element.pyx, you changed in one place

except TypeError:

to

except (TypeError, ValueError):

Is this change needed?

Apparently not; the last commit reverts it.

comment:42 Changed 7 years ago by pbruin

  • Status changed from needs_info to positive_review

comment:43 Changed 7 years ago by jdemeyer

  • Dependencies changed from #16507, #17578 to #16507, #17578, #17561
  • Status changed from positive_review to needs_work

There is a minor conflict with #17561.

comment:44 Changed 7 years ago by jdemeyer

  • Branch changed from u/pbruin/10513-coercion_and_categories_for_modules to u/jdemeyer/ticket/10513
  • Modified changed from 01/11/15 21:33:50 to 01/11/15 21:33:50

comment:45 Changed 7 years ago by jdemeyer

  • Commit changed from 99ec4cc13099658e0bc762a6e8d230c3956c7150 to b1287e75962a2dc590f1fa22acc90a4e53383aab
  • Milestone changed from sage-6.4 to sage-pending
  • Status changed from needs_work to positive_review

New commits:

df0fb69Declare types of _entries
106f5d4Minor improvements in FreeModuleElement_generic_sparse.__init__
b1287e7Merge branch 'ticket/17561' into ticket/10513

comment:46 Changed 7 years ago by nbruin

  • Status changed from positive_review to needs_work

On 6.5.beta5:

sage: %time for p in prime_range(10^5): V=(GF(p))^3
CPU times: user 3.63 s, sys: 28 ms, total: 3.66 s
Wall time: 3.65 s

With this branch:

sage: %time for p in prime_range(10^5): V=(GF(p))^3
CPU times: user 13.4 s, sys: 29 ms, total: 13.5 s
Wall time: 13.5 s

I think that's a significant regression in performance. It's probably due to the construction of the category-with-base. If you change the category to be "Category of vector spaces over Fields" or "over Finite Fields" you'll get much better performance, because there's not a new category to be constructed for every single space.

(this stuff isn't academic--such spaces really get made a lot in CRT code). See also #17360, where the issue causes a memleak (that seems to be fine here). See also http://trac.sagemath.org/ticket/10963#comment:506 where this issue was mentioned right at the start of the development of these parametrized categories.

comment:47 follow-up: Changed 7 years ago by nbruin

Yep, with

  • src/sage/modules/free_module.py

    diff --git a/src/sage/modules/free_module.py b/src/sage/modules/free_module.py
    index 4cf68fc..dc9248c 100644
    a b done from the right side.""") 
    721721
    722722        if category is None:
    723723            from sage.categories.all import Fields, FreeModules, VectorSpaces
    724             if base_ring in Fields():
    725                 category = VectorSpaces(base_ring)
     724            F=Fields()
     725            if base_ring in F:
     726                category = VectorSpaces(F)
    726727            else:
    727                 category = FreeModules(base_ring)
     728                category = FreeModules(base_ring.category())
    728729            try:
    729730                if base_ring.is_finite() or rank == 0:
    730731                    # Put the module in the category of finite enumerated

I get

sage: %time for p in prime_range(10^5): V=(GF(p))^3
CPU times: user 2.52 s, sys: 27 ms, total: 2.54 s
Wall time: 2.53 s

In profiling this example I'm still finding

    28774    0.054    0.000    0.329    0.000 category.py:2281(join)

so it seems we're still spending more than 10% of our time on dynamically constructing a category (taking the join with FiniteEnumeratedSets()) just to get a list method. This looks like another candidate to improve (a bit).

On the other hand, part of this might by due to GF(p) construction by itself. If I first do L=[GF(p) for p in prime_range(10^5)] the run time drops to 1.19 s, there are less calls to join (exactly one per constructed field, so GF(p) seems to need two category joins) and the impact of category.join drops to 0.086, which is about 6-7% of the run time then.

Last edited 7 years ago by nbruin (previous) (diff)

comment:48 in reply to: ↑ 47 ; follow-up: Changed 7 years ago by pbruin

Replying to nbruin:

On the other hand, part of this might by due to GF(p) construction by itself. If I first do L=[GF(p) for p in prime_range(10^5)] the run time drops to 1.19 s, there are less calls to join (exactly one per constructed field, so GF(p) seems to need two category joins) and the impact of category.join drops to 0.086, which is about 6-7% of the run time then.

Indeed, the category of GF(p) is a join of three categories:

sage: GF(3).category()
Join of Category of finite fields and Category of subquotients of monoids and Category of quotients of semigroups

Maybe parts of the category code (like join) should be transformed into Cython to make it faster?

comment:49 Changed 7 years ago by pbruin

  • Branch changed from u/jdemeyer/ticket/10513 to u/pbruin/10513-coercion_and_categories_for_modules
  • Commit changed from b1287e75962a2dc590f1fa22acc90a4e53383aab to 6b63026ceea5e9daaee46ca821a85e4edd6ded8b
  • Dependencies changed from #16507, #17578, #17561 to #16507, #17578, #17561, #11126

With the new commits, the category of free modules is FreeModules(base_ring.category()) by default, and instead of taking the join with FiniteEnumeratedSets (if applicable), we add a list() method.

There are two doctest failures:

sage -t --long --warn-long 43.7 src/sage/matrix/matrix2.pyx  # 1 doctest failed
sage -t --long --warn-long 43.7 src/sage/modules/free_module.py  # 1 doctest failed

The first is caused by #11126; the second is a NotImplementedError in repr() caused by the fact that basis() is not implemented for abstract free modules.


New commits:

f0957f0Trac 10513: use FreeModules(base_ring.category()) instead of FreeModules(base_ring)
6b63026Trac 10513: method FreeModule_generic.list() to replace FiniteEnumeratedSets magic

comment:50 in reply to: ↑ 48 Changed 7 years ago by nbruin

Replying to pbruin:

Indeed, the category of GF(p) is a join of three categories:

sage: GF(3).category()
Join of Category of finite fields and Category of subquotients of monoids and Category of quotients of semigroups

Maybe parts of the category code (like join) should be transformed into Cython to make it faster?

That's one option, but ultimately taking joins is a a rather dynamic operation that needs to investigate quite a bit of the category graph in order to ensure it's safe to use what's cached (can the result of a join depend on changes elsewhere in the graph?). In the case of finite fields, there is *no* variation in what the category is: this thing can be created up front (to accommodate GF(2), which is always present if I'm not mistaken), and just be set to be the category. The current problem is that the joining of categories happens in some of the more generic construction routines. We'd need a way of signalling that we want to shortcut that (a precise_category keyword that percolates up perhaps?). That's an optimization strategy that may have more pay-off for this particular scenario than making "join" faster (which might still be worthwhile to do). In any case, that's for another ticket. Also: premature optimization etc. Thanks for looking into this.

comment:51 Changed 7 years ago by git

  • Commit changed from 6b63026ceea5e9daaee46ca821a85e4edd6ded8b to ac7098684fe07997da64bacee522d88c5530c38a

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

6d5aaa1Merge branch 'develop' into ticket/10513-coercion_and_categories_for_modules
d1db7eaTrac 10513: construct coerce_map as default conversion map instead of matrix morphism
7b09742Trac 10513: checking if element_class is a method is no longer necessary
ac685ecTrac 10513: update doctests
ac70986Trac 10513: small documentation improvement

comment:52 Changed 7 years ago by pbruin

  • Status changed from needs_work to needs_review

Everything should work again after this update.

comment:53 follow-up: Changed 7 years ago by vdelecroix

  • Status changed from needs_review to needs_work

Not everything...

some tests failed:
sage -t --long src/sage/matrix/matrix2.pyx  # 1 doctest failed
sage -t --long src/sage/modular/modform/constructor.py  # 1 doctest failed
sage -t --long src/sage/modular/modform/eisenstein_submodule.py  # 1 doctest failed
sage -t --long src/sage/modular/modform/ambient_g1.py  # 1 doctest failed
sage -t --long src/sage/modular/arithgroup/congroup_gamma0.py  # 1 doctest failed

comment:54 in reply to: ↑ 53 ; follow-up: Changed 7 years ago by pbruin

Replying to vdelecroix:

Not everything...

some tests failed:
sage -t --long src/sage/matrix/matrix2.pyx  # 1 doctest failed
sage -t --long src/sage/modular/modform/constructor.py  # 1 doctest failed
sage -t --long src/sage/modular/modform/eisenstein_submodule.py  # 1 doctest failed
sage -t --long src/sage/modular/modform/ambient_g1.py  # 1 doctest failed
sage -t --long src/sage/modular/arithgroup/congroup_gamma0.py  # 1 doctest failed

I just tested again with 6.6.rc0 and #11126 merged, and got no failures. What kind of errors are you getting?

comment:55 in reply to: ↑ 54 Changed 7 years ago by vdelecroix

Replying to pbruin:

Replying to vdelecroix:

...

I just tested again with 6.6.rc0 and #11126 merged, and got no failures. What kind of errors are you getting?

Strange... I am launching the tests again.

comment:56 follow-up: Changed 7 years ago by jdemeyer

Please be aware of #17562 which also involves modules. Given that #10513 is mostly about the parent and #17562 is mostly about the elements, there is probably no conflict but I felt like I should mention this anyway.

comment:57 follow-up: Changed 7 years ago by vdelecroix

Results of the new test... most of the failures disappeared except one

sage -t --long src/sage/matrix/matrix2.pyx
**********************************************************************
File "src/sage/matrix/matrix2.pyx", line 1931, in sage.matrix.matrix2.Matrix.minpoly
Failed example:
    m.minimal_polynomial()
Exception raised:
    Traceback (most recent call last):
    ...
    NotImplementedError: is_squarefree() is only implemented for polynomials over principal ideal domains
**********************************************************************

comment:58 in reply to: ↑ 57 Changed 7 years ago by pbruin

Replying to vdelecroix:

Results of the new test... most of the failures disappeared except one

sage -t --long src/sage/matrix/matrix2.pyx
**********************************************************************
File "src/sage/matrix/matrix2.pyx", line 1931, in sage.matrix.matrix2.Matrix.minpoly
Failed example:
    m.minimal_polynomial()
Exception raised:
    Traceback (most recent call last):
    ...
    NotImplementedError: is_squarefree() is only implemented for polynomials over principal ideal domains
**********************************************************************

This one is fixed by #11126. (The other failures were probably fixed by #17578 which is in 6.6.rc0.) I deliberately did not merge #11126 into this branch since I expected some discussion about it. Maybe this particular failure can be fixed in a more minimalistic way than as part of #11126.

comment:59 Changed 7 years ago by pbruin

  • Dependencies changed from #16507, #17578, #17561, #11126 to #16507, #17578, #17561, #18040

comment:60 in reply to: ↑ 56 Changed 7 years ago by pbruin

  • Milestone changed from sage-pending to sage-6.6
  • Status changed from needs_work to needs_review

Replying to jdemeyer:

Please be aware of #17562 which also involves modules.

All tests pass after merging #17562 and the new dependency #18040.

comment:61 Changed 7 years ago by tscrim

  • Dependencies changed from #16507, #17578, #17561, #18040 to #16507, #17578, #17561, #18040, #17562

Is this then ready to go back to positive review? Vincent, did/could you run through the tests again?

comment:62 Changed 7 years ago by pbruin

Note: #17562 is actually independent of this ticket; it merges cleanly and tests pass with or without it. Also, I intentionally did not merge #18040 (or any other dependencies) into the branch here. The only merge conflict was with #17561, which was fixed by Jeroen.

comment:63 Changed 7 years ago by vdelecroix

At least on my computer

All tests passed!

comment:64 Changed 7 years ago by tscrim

  • Reviewers changed from Jean-Pierre Flori to Jean-Pierre Flori, Vincent Delecroix
  • Status changed from needs_review to positive_review

Thanks Vincent. I'm going to set this (back) to positive review.

comment:65 Changed 7 years ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict with 6.7.beta0

comment:66 Changed 7 years ago by git

  • Commit changed from ac7098684fe07997da64bacee522d88c5530c38a to 9865576ec5600a1502e5c5e455e5f2ce08406f4c

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

9865576Merge branch 'develop' into ticket/10513-coercion_and_categories_for_modules

comment:67 Changed 7 years ago by pbruin

  • Status changed from needs_work to positive_review

There was only a minor conflict; I assume this doesn't need another review.

comment:68 Changed 7 years ago by vbraun

  • Branch changed from u/pbruin/10513-coercion_and_categories_for_modules to 9865576ec5600a1502e5c5e455e5f2ce08406f4c
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.