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

Improved itermonomials #16561

Merged
merged 19 commits into from
May 6, 2019
Merged

Improved itermonomials #16561

merged 19 commits into from
May 6, 2019

Conversation

ctsiagkalis
Copy link
Contributor

@ctsiagkalis ctsiagkalis commented Apr 5, 2019

Brief description of what is fixed or changed

itermonomials now computes only the necessary monomials and returns a generator object

Release Notes

  • polys
    • itermonomials is now a generator.
    • itermonomials now accepts (besides the list of variables) either a list of degrees (one degree for each variable) or an integer (same degree for all the variables) for either the min_degrees and max_degrees parameters. min_degrees can also be omitted, in which case it corresponds to a 0 (single int) or a list of 0's.

@sympy-bot
Copy link

sympy-bot commented Apr 5, 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
    • itermonomials is now a generator. (#16561 by @ctsiagkalis)

    • itermonomials now accepts (besides the list of variables) either a list of degrees (one degree for each variable) or an integer (same degree for all the variables) for either the min_degrees and max_degrees parameters. min_degrees can also be omitted, in which case it corresponds to a 0 (single int) or a list of 0's. (#16561 by @ctsiagkalis)

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.

#### Brief description of what is fixed or changed

itermonomials now computes only the necessary monomials and returns a generator object


#### Release Notes

<!-- BEGIN RELEASE NOTES -->

* polys
    * itermonomials is now a generator.
    * itermonomials now accepts (besides the list of variables) either a list of degrees (one degree for each variable) or an integer (same degree for all the variables) for either the `min_degrees` and `max_degrees` parameters. `min_degrees` can also be omitted, in which case it corresponds to a 0 (single int) or a list of 0's.

<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

@jksuom
Copy link
Member

jksuom commented Apr 5, 2019

There is trailing whitespace that should be removed:

File contains trailing whitespace: /home/travis/build/sympy/sympy/sympy/polys/monomials.py, line 43.

This is the first error, there may be others. You can find them by running bin/test quality.

sympy/polys/monomials.py Outdated Show resolved Hide resolved
@asmeurer
Copy link
Member

asmeurer commented Apr 5, 2019

Please try to write instructive release notes. I updated it, but please check that what I wrote is correct and that I didn't miss anything.

@asmeurer
Copy link
Member

asmeurer commented Apr 5, 2019

Perhaps we should make the function a generator (with yield statements instead of return).

@ctsiagkalis
Copy link
Contributor Author

Thanks for all your comments and suggestions.
I updated itermonomials to return always a generator object (even in the trivial cases as you suggested), and also updated the release notes.
@asmeurer
I seem to have some issues with the tests. I tried running bin/test quality monomials.py or python bin/test quality monomials py but it always shows no error and passes the tests. Am I doing this wrong?

@jksuom
Copy link
Member

jksuom commented Apr 9, 2019

Changing the return type to generator has caused some failures. One of the places that have to be fixed is here

if A > 1 and B > 1:
monoms = itermonomials(V, A + B - 1 + degree_offset)
else:
monoms = itermonomials(V, A + B + degree_offset)

On the two lines, monoms should be defined as tuple(itermonomials(...)). That should take care of a number of errors. You can check them locally by running bin/test heurisch.

ctsiagkalis and others added 9 commits April 19, 2019 16:29
`min_degrees` and `max_degrees`.
Negative `max_degrees` raises `ValueError` instead of
returning an empty Set.
Split double conditions in `if` statements to multiple `if`'s.
objects, using `next()` and `set()`.
Also, added `raises()` for new `ValueError`'s in case of negative
`max_degrees`.
Some old examples didn't work with the generators implementation.
Fixed them forcing list/set to match each example's result.

There was also an error in line 89 while the online tester was parsing
the file and finding EOF. I added a "\" character to hopefully escape
the whitespace.
Removed extra comma causing error in tests from line 64.
The "\" character didn't work, so I merged the two lines.
Title underline in docstring of itermonomials was too short.
@codecov
Copy link

codecov bot commented Apr 20, 2019

Codecov Report

Merging #16561 into master will increase coverage by 0.038%.
The diff coverage is 93.846%.

@@             Coverage Diff              @@
##           master    #16561       +/-   ##
============================================
+ Coverage   73.83%   73.868%   +0.038%     
============================================
  Files         619       619               
  Lines      159454    159488       +34     
  Branches    37430     37446       +16     
============================================
+ Hits       117725    117811       +86     
+ Misses      36270     36224       -46     
+ Partials     5459      5453        -6

sympy/polys/monomials.py Outdated Show resolved Hide resolved
sympy/polys/monomials.py Outdated Show resolved Hide resolved
Replaced redundant `yield` statement with `return`.
Added return statement when returning a single element
making the generator terminating a bit faster.
sympy/polys/monomials.py Outdated Show resolved Hide resolved
@jksuom
Copy link
Member

jksuom commented Apr 24, 2019

Can you add tests for the degree list cases as well?

@jksuom
Copy link
Member

jksuom commented May 5, 2019

The new test examples look good. One change would still be needed: yield {S(1)} should be yield S(1), that is, it should return the number S(1) instead of the set {S(1}. The relevant examples should then work with set(...) instead of next(...).

@ctsiagkalis
Copy link
Contributor Author

ctsiagkalis commented May 5, 2019

The new test examples look good. One change would still be needed: yield {S(1)} should be yield S(1), that is, it should return the number S(1) instead of the set {S(1}. The relevant examples should then work with set(...) instead of next(...).

You are right. I'll change it for consistency in the tests. I will also change lines 38-40 in test_monomials.py to use set() instead of next().

EDIT:
Messed up the commit message so i force-pushed to change it.

In `total_degree` case if the `variables` list is empty or
the `max_degree` is 0 `S(1)` should be returned instead of `{S(1)}`.
Also updated the tests to support the above change.
@jksuom
Copy link
Member

jksuom commented May 6, 2019

I think this is ready to be merged. The old total degree part could probably be implemented better as a generator but that is not necessary in this PR. Thank you for your contribution.

@jksuom jksuom merged commit 8dcb72f into sympy:master May 6, 2019
@ctsiagkalis
Copy link
Contributor Author

I think this is ready to be merged. The old total degree part could probably be implemented better as a generator but that is not necessary in this PR. Thank you for your contribution.

I am glad I helped. This was my first PR and I have to admit that you helped me very much. Thank you for everything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants