Opened 3 years ago
Closed 3 years ago
#27421 closed enhancement (fixed)
.coefficient of multivariate polynomial should accept output of .exponents()
Reported by:  dkrenn  Owned by:  

Priority:  minor  Milestone:  sage8.7 
Component:  algebra  Keywords:  easy, beginner 
Cc:  ghChamanAgrawal  Merged in:  
Authors:  Chaman Agrawal  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  f5e3eb4 (Commits, GitHub, GitLab)  Commit:  f5e3eb47b1f08fafcc9578327eab74c84be8491c 
Dependencies:  Stopgaps: 
Description
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.
Change History (13)
comment:1 Changed 3 years ago by
 Branch set to u/ghChamanAgrawal/27421_coefficient
 Cc ghChamanAgrawal added
 Commit set to c5c4e8185d3c266e549e7b438f53417095e37330
 Status changed from new to needs_review
comment:2 Changed 3 years ago by
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() ).
comment:3 Changed 3 years ago by
 Commit changed from c5c4e8185d3c266e549e7b438f53417095e37330 to a406d1ec194bac92656be1c450234b4bf2a20b92
Branch pushed to git repo; I updated commit sha1. New commits:
a406d1e  Added Doctest for #27421

comment:4 Changed 3 years ago by
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 oldstyle Cython declarations
for i from 0<=i<gens: +for i in range(gens):
comment:5 Changed 3 years ago by
 Commit changed from a406d1ec194bac92656be1c450234b4bf2a20b92 to 5c5fe615439bc6dbb8f45250dace2074224e1681
Branch pushed to git repo; I updated commit sha1. New commits:
5c5fe61  Using ETuple directly instead of converting to list

comment:6 Changed 3 years ago by
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 overindented.
Thanks.
comment:7 Changed 3 years ago by
 Commit changed from 5c5fe615439bc6dbb8f45250dace2074224e1681 to db2f0458b3e6b96b6d8ce032bb1d106256657dc4
Branch pushed to git repo; I updated commit sha1. New commits:
db2f045  Removed over indentation and added equivalence test

comment:8 followup: ↓ 9 Changed 3 years ago by
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
comment:9 in reply to: ↑ 8 Changed 3 years ago by
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.
comment:10 Changed 3 years ago by
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
comment:11 Changed 3 years ago by
 Commit changed from db2f0458b3e6b96b6d8ce032bb1d106256657dc4 to f5e3eb47b1f08fafcc9578327eab74c84be8491c
Branch pushed to git repo; I updated commit sha1. New commits:
f5e3eb4  Modified DocTests

comment:12 Changed 3 years ago by
 Reviewers set to Travis Scrimshaw
 Status changed from needs_review to positive_review
Thanks. LGTM.
comment:13 Changed 3 years ago by
 Branch changed from u/ghChamanAgrawal/27421_coefficient to f5e3eb47b1f08fafcc9578327eab74c84be8491c
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Handle ETuple in coefficient()