Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature power_list to return all powers of a variable present in f #17043

Closed
wants to merge 14 commits into from

Conversation

Projects
None yet
6 participants
@jmig5776
Copy link
Member

commented Jun 17, 2019

References to other Issues or PRs

Brief description of what is fixed or changed

A new feature power_list to return powers of variables present in expr.

>>>Poly(x**2*y + x**3*z**2 + 1).power_list()
[(3, 2, 0), (1, 0), (2, 0)]
>>>eq = x**3+x**2*y**3+x+1
>>>power_list(eq, y)
[(3, 0)]

Other comments

@aktech @Yathartha22 @Shekharrajak @smichr @asmeurer

Release Notes

  • polys
    • added new feature power_list to return powers of a symbol present in expression.
@sympy-bot

This comment has been minimized.

Copy link

commented Jun 17, 2019

Hi, I am the SymPy bot (v147). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

  • polys
    • added new feature power_list to return powers of a symbol present in expression. (#17043 by @jmig5776)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.5.

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

Click here to see the pull request description that was parsed.

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234". See
https://github.com/blog/1506-closing-issues-via-pull-requests . Please also
write a comment on that issue linking back to this pull request once it is
open. -->


#### Brief description of what is fixed or changed
A new feature `power_list` to return powers of variables present in expr.
```python
>>>Poly(x**2*y + x**3*z**2 + 1).power_list()
[(3, 2, 0), (1, 0), (2, 0)]
>>>eq = x**3+x**2*y**3+x+1
>>>power_list(eq, y)
[(3, 0)]
```

#### Other comments
@aktech @Yathartha22 @Shekharrajak @smichr @asmeurer 

#### Release Notes

<!-- Write the release notes for this release below. See
https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more information
on how to write release notes. The bot will check your release notes
automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
* polys
  * added new feature `power_list` to return powers of a symbol present in expression.
<!-- END RELEASE NOTES -->

@jmig5776

This comment has been minimized.

Copy link
Member Author

commented Jun 17, 2019

Please let me know if my approach is right to create this new feature.

Show resolved Hide resolved sympy/polys/densebasic.py Outdated
Show resolved Hide resolved sympy/polys/densebasic.py Outdated
return [0]
else:
for i in range(len(f)):
if f[i] != 0:

This comment has been minimized.

Copy link
@czgdp1807

czgdp1807 Jun 18, 2019

Member

May be, AFAICFO, this will return the powers of constants too? Please correct me if I am wrong.

This comment has been minimized.

Copy link
@jmig5776

jmig5776 Jun 18, 2019

Author Member

No this function will only return power of symbols present in expression. You can go through the tests and can also verify running this PR locally.

Show resolved Hide resolved sympy/polys/polytools.py Outdated
"""
if hasattr(f.rep, 'power_list'):
return f.rep.power_list()
else: # pragma: no cover

This comment has been minimized.

Copy link
@czgdp1807

czgdp1807 Jun 18, 2019

Member

else not needed.

This comment has been minimized.

Copy link
@jmig5776

jmig5776 Jun 18, 2019

Author Member

Was based on old code structured followed but i removed else in next commits to improve code quality in next commits.
May be you can open a PR to remove unnecessary else written in polytools file to improve code quality.

Show resolved Hide resolved sympy/polys/densebasic.py Outdated
@codecov

This comment has been minimized.

Copy link

commented Jun 18, 2019

Codecov Report

Merging #17043 into master will increase coverage by 0.019%.
The diff coverage is 100%.

@@              Coverage Diff              @@
##            master    #17043       +/-   ##
=============================================
+ Coverage   74.412%   74.432%   +0.019%     
=============================================
  Files          622       622               
  Lines       160838    160944      +106     
  Branches     37752     37785       +33     
=============================================
+ Hits        119684    119795      +111     
- Misses       35829     35831        +2     
+ Partials      5325      5318        -7
@Yathartha22

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

Good work! Although I hoped here (#16890 (comment)) to have a lower level function in bivariate itself that would do what we needed. Nevertheless I would like to have @jksuom's view point on this routine whether it will be good idea to have such a thing for polynomials.

@jksuom

This comment has been minimized.

Copy link
Member

commented Jun 19, 2019

Would it be possible to collect the powers from the output of monoms? The code might be simplified as monoms takes care of the recursion internally.

@jmig5776

This comment has been minimized.

Copy link
Member Author

commented Jun 19, 2019

@jksuom I get your point. I will try to implement it in next commits soon.

@jmig5776

This comment has been minimized.

Copy link
Member Author

commented Jun 19, 2019

@jksuom Please review and let me know what you think.

@jksuom

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

The docstring does not say it explicitly but it looks like the degrees of different variables all appear mixed in a single list. I don't know of any application where that could be used. Have you got some very specific purpose in mind?

@jmig5776

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2019

@jksuom As mentioned in #16890 (comment) we required there a routine which could return powers of symbols present in the expression. So that we could extract out even degrees of symbols present in the expression if present. I might had created a function their using monoms but if I was going to create a function their then I think it is better to create a general function in polytools. Because If someone might need to manipulate a polynomial with the powers of symbols present in them then he can easily do with this function.
Some might need to extract powers of symbols present in expression following a pattern.
This function also returns powers of specific symmbols if called by power_list(f, sym). This will return only powers of symbol present in expression.

@jmig5776

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2019

How it is different from monoms?
It returns a collective list of powers present unlike monoms which returns powers of terms which I think user will find difficult to iterate if they only have concern with powers of symbol present in them rather than the terms present.

for i in f.monoms():
power += list(i)
power = reversed(list(set(power)))
return tuple(power)

This comment has been minimized.

Copy link
@smichr

smichr Jun 20, 2019

Member

How will the user make sense of the result when there is more than one generator? If you look at degree I think you will see how we tried to handle that situation there. I think the approach is to allow no generator to be specified when the expression is univariate but to raise an error if no generator is given for the multivariate case.

This comment has been minimized.

Copy link
@jmig5776

jmig5776 Jun 20, 2019

Author Member

Okay I get your point. I will make this function work only when then generator is univariate. Can you please suggest how could I check if it is univariate and what kind of error should be raised.

@smichr

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

So that we could extract out even degrees of symbols present in the expression if present.

Do you just want to know if the expression is even wrt some gen? Won't eq.subs(gen, x) == eq.subs(gen, -x) where x is some dummy symbol tell you this?

@jmig5776

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2019

Do you just want to know if the expression is even wrt some gen?

No the whole point was that suppose an even degree of symbol is present in an expression like in (1/x + exp(x/2)).diff(x) so what was _lambert doing was converting x**2 + something taking log and converting to 2*log(x) + something_else but clearly x**2 can also be converted to 2log(-x) + something_else which was missed by _lambert.

Won't eq.subs(gen, x) == eq.subs(gen, -x) where x is some dummy symbol tell you this?

This was not helpful and even wrong. This only tells me if the function is even or not. I don't want that.
I am not only considering if only even degrees are present I am considering that even degrees are present doest matter odd degrees are present or not. eq.subs(gen, x) == eq.subs(gen, -x) this will give false when odd degrees are also present.

So what I did in #16890 is that I found out even powers of symbol present for which this PR is for and then convert the even_degrees to dummy symbol('t') so that I can differentiate between other and this. and then when all log operations had been applied then I replace t with +x and -x to give desired (not missed ) equations.

@jmig5776

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 2019

@smichr I had tried to make the output api more usefull to the user. Please review.

>>> Poly(x**2 + y*x + 1, x, y).power_list() # multivariate case returns powers in tuples
[(2, 1, 0), (1, 0)]
>>> Poly(x**2 + y*x + 1, x).power_list() # univariate case returns powers of only that gen
[(2, 1, 0)]
@jmig5776

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 2019

@smichr I had removed the comments . Can you please merge it after build is verified?


powers = F.power_list()

return list(powers)

This comment has been minimized.

Copy link
@smichr

smichr Jun 21, 2019

Member

In keeping with degree_list method vs function, the function return values should be sympified, so return sympify(list(powers)) will return Tuples of Integers.

@jksuom , what do you think about adding this function/method? I think the ambiguous output has been resolved and the function will find use at least in the Lambert work that is taking place. It may be of pedagogical interest, too.

@jksuom

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

what do you think about adding this function/method?

I cannot think of any case in polynomial algebra where monoms would not suffice give all data on the degrees. The use of degree_list in the Lambert solver seems very special to me. Perhaps it could be a helper function in that module.

@smichr

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

Perhaps it could be a helper function in that module.

That sounds good to me. That way, if someone figures out a better way to handle the Lambert solutions, the function will automatically be removed, otherwise it may become an "appendix" for Polys.

@jmig5776

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 2019

@jksuom Okay I will close this PR and make this function as helper function in bivariate. Also @smichr thanks for your valued input.

@jmig5776 jmig5776 closed this Jun 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.