Opened 11 years ago

Closed 7 years ago

Coercion and category framework for modules

Reported by: Owned by: SimonKing robertwb major sage-6.6 coercion coercion, category framework, modules vbraun, novoselt, jpflori Simon King, Peter Bruin Jean-Pierre Flori, Vincent Delecroix N/A 9865576 9865576ec5600a1502e5c5e455e5f2ce08406f4c #16507, #17578, #17561, #18040, #17562

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`.

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

comment:2 follow-up: ↓ 3 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

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: ↓ 25 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:

 ​5126b66 `Trac 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:

 ​ddda401 `Merge branch 'develop' into ticket/10513-coercion_and_categories_for_modules`

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:

 ​2bb997b `Merge branch 'develop' into ticket/10513-coercion_and_categories_for_modules` ​ed6bbf1 `Trac 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)
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.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:

 ​a19e2c5 `Merge branch 'develop' into ticket/10513-coercion_and_categories_for_modules` ​2f52a3c `Trac 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:

 ​2d7f075 `Trac 10513: small documentation improvement`

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

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

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: ↓ 27 Changed 7 years ago by pbruin

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

Thanks!

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: ↓ 28 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 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

comment:30 follow-up: ↓ 32 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

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: ↓ 36 Changed 7 years ago by jdemeyer

I think it's because of #10962.

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

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

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

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: ↓ 41 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:

 ​99ec4cc `Trac 10513: revert a change in try...except`

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

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:

 ​df0fb69 `Declare types of _entries` ​106f5d4 `Minor improvements in FreeModuleElement_generic_sparse.__init__` ​b1287e7 `Merge 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.

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 done from the right side.""") if category is None: from sage.categories.all import Fields, FreeModules, VectorSpaces if base_ring in Fields(): category = VectorSpaces(base_ring) F=Fields() if base_ring in F: category = VectorSpaces(F) else: category = FreeModules(base_ring) category = FreeModules(base_ring.category()) try: if base_ring.is_finite() or rank == 0: # 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: ↓ 50 Changed 7 years ago by pbruin

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:

 ​f0957f0 `Trac 10513: use FreeModules(base_ring.category()) instead of FreeModules(base_ring)` ​6b63026 `Trac 10513: method FreeModule_generic.list() to replace FiniteEnumeratedSets magic`

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

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:

 ​6d5aaa1 `Merge branch 'develop' into ticket/10513-coercion_and_categories_for_modules` ​d1db7ea `Trac 10513: construct coerce_map as default conversion map instead of matrix morphism` ​7b09742 `Trac 10513: checking if element_class is a method is no longer necessary` ​ac685ec `Trac 10513: update doctests` ​ac70986 `Trac 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: ↓ 54 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: ↓ 55 Changed 7 years ago by pbruin

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

...

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: ↓ 60 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: ↓ 58 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

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

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:

 ​9865576 `Merge 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.