make map_coefficients in modules_with_basis safer or more general

[mantepse]

We would love to be able to, for example, present a symmetric function with factored coefficients. But:

sage: f = SymmetricFunctions(ZZ).s()([2,2,1])
sage: f.map_coefficients(factor)
0

The result is not against the specification, because map_coefficients requires an endofunction.

There are two issues:

1.) we possibly want to be warned when supplying map_coefficients with something that is not an endofunction. This might be achieved by adding an optional argument check with default True.

2.) we want some way to factor the coefficients. Nicolas proposed to make something like

sage: f.map_coefficients(factor, codomain=SR)

or

sage: f.change_base_ring(SR).map_coefficients(factor) 

work.

Note that the Polynomial class contains something similar, namely:

def map_coefficients(self, f, new_base_ring=None)

However, there might be another tiny problem: it seems that currently it is not possible to make factored integers into a commutative ring. This should possibly be a separate ticket.

CC: @zabrocki @darijgr @sagetrac-sage-combinat @hivert

Component: algebra

Keywords: map_coefficients, modules_with_basis, base_ring, check arguments

Author: Martin Rubey

Branch/Commit: public/ticket/18264 @ d0d80e7

Reviewer: Nicolas M. Thiéry

Issue created by migration from https://trac.sagemath.org/ticket/18264

[mantepse]

Branch: u/mantepse/make_map_coefficients_in_modules_with_basis_safer_or_more_general

[mantepse]

comment:3

Doing this I noticed that map_coefficients did not make use of the distinct feature of sum_of_terms. I added this too, assuming that initially an element has distinct monomials.

[mantepse]

Commit: 3a7761e

[mantepse]

Author: Martin Rubey

[nthiery]

comment:7

Hi Martin!

Thanks for taking action; this is looking good!

Now that I am reading actual code: a usual idiom to check that a
coefficient is indeed in the base ring is to coerce it back there with
R(c). I guess that would be a fine behavior: it's faster, and it
takes care of all cases where someone passes an int, or something that
requires some small coercion to be put back in the base ring.

Maybe the option shall be named coerce after all. Comments anyone?

Note by the way that coercing would fail in Mike's original use case,
so whether we go for coerce or check don't make a difference
there:

sage: ZZ(factor(4))
TypeError: unable to coerce <class 'sage.structure.factorization_integer.IntegerFactorization'> to an integer

If we go for the coerce way, then the simplest would be to pass this
option down to sum_of_terms. This requires updating
CombinatorialFreeModule.sum_of_terms to implement that option, by
passing it down to respectively CombinatorialFreeModule._from_dict
where it's already implemented and CombinatorialFreeModule.term
where it would need to be implemented by passing the option down to
_from_dict.

It's getting a bit bigger of a change than what was planned for
originaly; do you feel like handling it?

Cheers,
Nicolas

[mantepse]

comment:9

Replying to @nthiery:

If we go for the coerce way, then the simplest would be to pass this
option down to sum_of_terms. This requires updating
CombinatorialFreeModule.sum_of_terms to implement that option, by
passing it down to respectively CombinatorialFreeModule._from_dict
where it's already implemented and CombinatorialFreeModule.term
where it would need to be implemented by passing the option down to
_from_dict.

I agree and will do that as soon as my sage works again (see sage-devel thread...)

[mantepse]

comment:10

Hi there!

I was just about to do what Nicolas suggest -- adding an optional argument coerce, but I encountered some things that make me hesitate:

1. to be helpful for the casual user, it's default value would have to be True - differently from what we have in _from_dict.

2. the docstring would then be something like

whether to try to coerce the result of applying `f` to the coefficient ring.

which is a bit strange, given that we require `f` to be an endofunction.

If you reassure me, I'll do it. I'm just not 100% sure myself.

[nthiery]

comment:11

Replying to @mantepse:

1. to be helpful for the casual user, it's default value would have to be True

• differently from what we have in _from_dict.

Yeah, I agree; it's not supper consistent. Yet _from_dict is a
private method when this one is public; so it's not unreasonable
either that the typical usage situation differ, inducing different
default behavior.

2. the docstring would then be something like

whether to try to coerce the result of applying `f` to the coefficient ring.

which is a bit strange, given that we require `f` to be an endofunction.

I see your point. We should be able to find some wording that makes it
sound natural. Maybe something like:

Whether to systematically coerce to the coefficient ring the result of applying f, for additional safety.

From my perspective you can proceed! Feedback anyone else?

Cheers,

[mantepse]

comment:12

Done (currently testing).

A question: in general, what is the requirement on the coefficients (morally, not enforced...)?

1. they should be in the base ring
2. they should be coercible to the base ring
3. you shouldn't expect anything...

1: In the internal representation, the coefficients are supposed to be
in the base ring and non zero (as tested by bool(c) aka
c.__nonzero__()).

I indeed would not be surprised if there were quite a few places in
the Sage sources where those assumptions are broken / abused. It would
be good to progressively detect and fix them. A step in this direction
is to find a better speed/safety/flexibility balance for when to run
sanity checks, or not, as you are doing here.

[mantepse]

comment:13

As it turns out, I get several failures when I insist on trying to coerce to the base ring! (I pushed the nonworking branch, to get feedback.) For example:

File "src/sage/combinat/ncsf_qsym/ncsf.py", line 3405, in sage.combinat.ncsf_qsym.ncsf.NonCommutativeSymmetricFunctions.Psi
Failed example:
    test_psi(2)
    ...
    TypeError: no conversion of this rational to integer

The test which fails looks as follows:

sage: NCSF = NonCommutativeSymmetricFunctions(ZZ)
sage: R = NCSF.R()
sage: Psi = NCSF.Psi()
sage: n = 2; a = R.sum([(-1) ** i * R[[1]*i + [n-i]] for i in range(n)])
sage: Psi(a)

The expected output is Psi[2] (literally). The easy fix would be to add the optional argument coerce=False to the calls of sum_of_terms in ncsf.py everywhere, but I'm not sure whether this is what I should do - because that would mean that the answer to the question in my previous comment is "3".

Martin

[nthiery]

comment:14

Replying to @mantepse:

As it turns out, I get several failures when I insist on trying to coerce to the base ring! (I pushed the nonworking branch, to get feedback.) For example:

Ok. Can you post here the test summary with the list of failing files,
just to get an idea of how bad this is?

File "src/sage/combinat/ncsf_qsym/ncsf.py", line 3405, in sage.combinat.ncsf_qsym.ncsf.NonCommutativeSymmetricFunctions.Psi
Failed example:
    test_psi(2)
    ...
    TypeError: no conversion of this rational to integer

The test which fails looks as follows:

sage: NCSF = NonCommutativeSymmetricFunctions(ZZ)
sage: R = NCSF.R()
sage: Psi = NCSF.Psi()
sage: n = 2; a = R.sum([(-1) ** i * R[[1]*i + [n-i]] for i in range(n)])
sage: Psi(a)

The expected output is Psi[2] (literally). The easy fix would be to add the optional argument coerce=False to the calls of sum_of_terms in ncsf.py everywhere, but I'm not sure whether this is what I should do - because that would mean that the answer to the question in my previous comment is "3".

If some intermediate calculations involve rational coefficients, then
we definitely want a failure here. Maybe there is a way to improve the
ncsf code to better support NCSF over integers by sticking to integral
computations.

Darij, Mike, ..., what do you think? Do you have a quick way to improve NCSF's
handling of integral coefficients? Or is it ok to switch this test to
QQ or mark it as "not implemented" for now?

[nthiery]

Reviewer: Nicolas M. Thiéry

[mantepse]

comment:16

I think it's only

2 items had failures:
   3 of  21 in sage.combinat.ncsf_qsym.ncsf.NonCommutativeSymmetricFunctions.Phi.Element.verschiebung
   3 of   8 in sage.combinat.ncsf_qsym.ncsf.NonCommutativeSymmetricFunctions.Psi
    [715 tests, 6 failures, 28.15 s]
----------------------------------------------------------------------
sage -t --warn-long 97.2 src/sage/combinat/ncsf_qsym/ncsf.py  # 6 doctests failed
----------------------------------------------------------------------

[mantepse]

comment:17

Out of curiosity, I modified _from_dict to to always try coercion. I then get some more failures:

----------------------------------------------------------------------
sage -t --warn-long 74.6 src/sage/modular/modform_hecketriangle/hecke_triangle_group_element.py  # Timed out
sage -t --warn-long 74.6 src/sage/algebras/steenrod/steenrod_algebra.py  # 1 doctest failed
sage -t --warn-long 74.6 src/sage/combinat/ncsf_qsym/ncsf.py  # 6 doctests failed
sage -t --warn-long 74.6 src/sage/combinat/sf/macdonald.py  # 4 doctests failed
sage -t --warn-long 74.6 src/sage/combinat/free_module.py  # 1 doctest failed
----------------------------------------------------------------------

[zabrocki]

comment:18

Darij put some effort into making sure that ncsf and qsym worked over the integers in obvious ways. I believe that this involved a lot of workarounds.

I am concerned about the conflicts with macdonald.py. I want to check those out carefully because we are in the process of fine tuning that code in #18194

IMO: I think that if you want to allow factor coefficients then the answer to your question is 3) you shouldn't expect anything...

[darijgr]

comment:19

Can you tell me how to exactly reproduce the failing doctests in ncsf.py? I don't have time for the rest, but at least I should see what's going wrong in my code. The test_psi(n) tests, though, have every right to fail. They should be fixed by replacing Psi(a) == Psi[n] by a == R(Psi[n]).

[sagetrac-git]

Changed commit from 3a7761e to 985e716

[sagetrac-git]

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

985e716

pass coerce=True down to _from_dict

[mantepse]

comment:21

OOps, I thought I pushed, but I actually only pushed just now.

Darij, if you pull the current branch, you'll see the failures.

[mantepse]

comment:22

Replying to @zabrocki:

IMO: I think that if you want to allow factor coefficients then the answer to your question is 3) you shouldn't expect anything...

I don't think so. A neat trick we should copy from FriCAS is to have a constructor Factored that takes a ring and returns a ring, keeping the elements in factored form.

So far, however, I do not know how to create a ring in sage...

[mantepse]

comment:23

I investigated the failure in sage.algebras.steenrod.steenrod_algebra.SteenrodAlgebra_generic.coproduct, which is actually not a real failure:

Failed example:
    SteenrodAlgebra(p=11, profile=((), (2,1,2))).Q(0,2).coproduct()
Expected:
    1 # Q_0 Q_2 + Q_0 # Q_2 + Q_0 Q_2 # 1 - Q_2 # Q_0
Got:
    1 # Q_0 Q_2 + Q_0 # Q_2 + Q_0 Q_2 # 1 + 10*Q_2 # Q_0

The p=11 in the example means that the coefficient ring is the field with 11 elements, so -1 and 10 are actually the same thing.

[darijgr]

Changed branch from u/mantepse/make_map_coefficients_in_modules_with_basis_safer_or_more_general to public/ticket/18264

[darijgr]

Changed commit from 985e716 to d0d80e7

[darijgr]

New commits:

6e52f39	Merge branch 'u/mantepse/make_map_coefficients_in_modules_with_basis_safer_or_more_general' of git://trac.sagemath.org/sage into map
d0d80e7	fix ncsf.py and steenrod_algebra.py (both times the doctests were bad)

[darijgr]

comment:25

I've fixed the doctests in two files.

The doc failure in macdonald.py seems a real issue. Mike? (Anyone who works on this needs the newest develop version, 6.7.beta3, since the Macdonald functions have undergone a major change in this version.)

Also the error in free_module.py itself seems to suggest that some optional parameters are being ignored...

[mantepse]

comment:26

Replying to @darijgr:

I've fixed the doctests in two files.

Thank you!

Also the error in free_module.py itself seems to suggest that some optional parameters are being ignored...

Yes, as I mentioned above (I hope), I "manually" set coerce = True in _from_dict to catch errors.

[darijgr]

comment:27

Oh! Does this mean I've built on top of an experimental branch? Sorry!

[mantepse]

comment:28

I'm sorry, I did not warn explicitely enough.

To get a production branch, simply remove the line coerce = True in _from_dict and recompile.

[darijgr]

comment:29

The doctest fixes are now in #19771.

[mezzarobba]

comment:30

Another case where the implementation of map_coefficients in module_with_basis is called but doesn't work:

sage: vector([1,2]).map_coefficients(lambda x: x+1)
...
TypeError: 'sage.rings.integer.Integer' object is not iterable