.coefficient of multi-variate polynomial should accept output of .exponents()

dkrenn commented 5 years ago

sage: P.<x,y> = QQ[]
sage: x.coefficient(x.exponents()[0])

gives

TypeError: The input degrees must be a dictionary of variables to exponents.

and one has to use

sage: x.coefficient(list(x.exponents()[0]))
1

as a workaround. However, it seems strange, that the exponents output by the same object are not accepted as input to another method.

CC: @ChamanAgrawal

Component: algebra

Keywords: easy

Author: Chaman Agrawal

Branch/Commit: f5e3eb4

Reviewer: Travis Scrimshaw

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

ChamanAgrawal commented 5 years ago

New commits:

c5c4e81 Handle ETuple in coefficient()

ChamanAgrawal commented 5 years ago

Author: Chaman Agrawal

ChamanAgrawal commented 5 years ago

Branch: u/gh-ChamanAgrawal/27421_coefficient

ChamanAgrawal commented 5 years ago

Commit: c5c4e81

ChamanAgrawal commented 5 years ago

comment:2

This is my first time in sagemath so sorry if I have missed some important community standards.

For the issue I thought two possible solutions:

1) Change the return type of exponents() to list.

2) Change coefficient() to handle the return value of exponents()

Changing the return type of exponents() will affect all the dependent functions of exponents() so I have changed coefficient() to handle ETuple also ( return type of exponents() ).

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

a406d1e Added Doctest for #27421

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from c5c4e81 to a406d1e

tscrim commented 5 years ago

comment:4

if degrees_list is None:

is vacuous because list(foo) should never return None. Also, it seems wasteful to convert it to a list. Why not just use the ETuple directly with

exps[i] = int((<ETuple>degrees).get_exp(i))

Also, we want to move away from the old-style Cython declarations

-for i from 0<=i<gens:
+for i in range(gens):

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

5c5fe61 Using ETuple directly instead of converting to list

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from a406d1e to 5c5fe61

tscrim commented 5 years ago

comment:6

Last two little things:

1 - You should test f.coefficient(x.exponents()[0]) to show that the output matches the above (i.e., it is an equivalent input). 2 - Your line exps[i] = int((<ETuple>degrees).get_exp(i)) is over-indented.

Thanks.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from 5c5fe61 to db2f045

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

db2f045 Removed over indentation and added equivalence test

tscrim commented 5 years ago

comment:8

No, that is not what I wanted for the test. I was looking for this change

-            sage: x.coefficient(x.exponents()[0])
-            1
+            sage: f.coefficient(x.exponents()[0])
+            y^2 + y + 1

ChamanAgrawal commented 5 years ago

comment:9

Replying to @tscrim:

No, that is not what I wanted for the test. I was looking for this change
-            sage: x.coefficient(x.exponents()[0])
-            1
+            sage: f.coefficient(x.exponents()[0])
+            y^2 + y + 1

But that is not how exponents() works. In the Multivariate Polynomial Ring in x, y over Rational Field R.<x,y> = QQ[] x has exponent (1,0) so x.exponents() returns [(1, 0)] and so f.coefficient(x.exponents()[0]) returns 1.

I don't think changing the behavior of exponents() is the right course of action, So can you please explain further the desired behaviour of the coefficient() in this case.

tscrim commented 5 years ago

comment:10

Ah, right, it has the y value explicitly set. Okay, then let's change the second part of your test to make it more clear what is going on, with a bit more explanation:

            sage: f.coefficient(x)
            y^2 + y + 1

        Note that exponents have all variables specified::

            sage: x.coefficient(x.exponents()[0])
            1
            sage: f.coefficient([1,0])
            1
            sage: f.coefficient({x:1,y:0})
            1

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from db2f045 to f5e3eb4

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

f5e3eb4 Modified DocTests

tscrim commented 5 years ago

comment:12

Thanks. LGTM.

tscrim commented 5 years ago

Reviewer: Travis Scrimshaw

vbraun commented 5 years ago

Changed branch from u/gh-ChamanAgrawal/27421_coefficient to f5e3eb4

sagemath / sage

.coefficient of multi-variate polynomial should accept output of .exponents() #27421