Modified powerset function in EFS to reduce memory consumption

[admercs]

Description

I modified the powerset function and calculation of number of possible combinations to reduce memory overhead by not longer storing all combinations as a list.

Related issues or pull requests

Fixes # #194

Pull Request requirements

• Added appropriate unit test functions in the ./mlxtend/*/tests directories
• Ran nosetests ./mlxtend -sv and make sure that all unit tests pass
• Checked the test coverage by running nosetests ./mlxtend --with-coverage
• Checked for style issues by running flake8 ./mlxtend
• Added a note about the modification or contribution to the ./docs/sources/CHANGELOG.md file
• Modify documentation in the appropriate location under mlxtend/docs/sources/ (optional)
• Checked that the Travis-CI build passed at https://travis-ci.org/rasbt/mlxtend

[pep8speaks]

Hello @adam-erickson! Thanks for updating the PR.

• In the file mlxtend/feature_selection/exhaustive_feature_selector.py, following are the PEP8 issues :

Line 163:27: E128 continuation line under-indented for visual indent
Line 167:1: W293 blank line contains whitespace
Line 170:1: W293 blank line contains whitespace
Line 177:1: W293 blank line contains whitespace
Line 181:1: W293 blank line contains whitespace
Line 183:1: W293 blank line contains whitespace
Line 190:1: W293 blank line contains whitespace
Line 194:1: W293 blank line contains whitespace

Comment last updated on May 21, 2017 at 17:03 Hours UTC

[rasbt]

Thanks for the PR! I just gave it a quick glance and there's already something like ncr (math.num_combinations); prob a bit more efficient since it doesn't evaluate range objects.

E.g., you could replace

all_comb = np.sum([ncr(n=X.shape[1], r=i)
                   for i in range(self.min_features,
                                  self.max_features + 1)])

by

from ..math import num_combinations
...
all_comb = np.sum([num_combinations(n=X.shape[1], k=i)
                   for i in range(self.min_features,
                                  self.max_features + 1)])

[admercs]

Supposedly, the implementation provided is ~25% more efficient than math.factorial: http://stackoverflow.com/questions/4941753/is-there-a-math-ncr-function-in-python

Update: I tested both on (n=100, r=10) and the one I provided was ~ 20% more efficient. I ran tests again on (n=10, r=5) and found the opposite, as your implementation was 3x faster here. However, the below version was twice as fast as yours for small datasets and also faster for large datasets:

def nCr(n,r):
    return factorial(n) // factorial(r) // factorial(n-r)

I'd consider this version optimal.

[rasbt]

Interesting! However, I think any of these functions runs so quickly (compared to the rest) that speed is really not a concern here. Sorry my previous comment was a bit misleading; I primarily thought that resusing the existing num_combinations function would be a good idea to avoid reimplementing this code here (in the sense of refactoring or duplication of effort :)).

[rasbt]

Hi, @adam-erickson . Just revisiting this discussion from last weekend ...
That's a really good fix. and I'd like to merge it. Can we use the internal from ..math import num_combinations for this though to keep the code simple and add a short message to the Changelog? I am happy to make those changes via "Allow edits from maintainers" if you don't mind :)

[admercs]

Hi @rasbt sorry for the slow reply, I was releasing gapfraction and its website. Of course, we can use the num_combinations function if you'd like for simplicity. What can I do to help facilitate these changes? I'm a bit new to this process. Should I close the pull request?

[rasbt]

No worries at all -- just wasn't sure if you'd okay with the proposed changes.

What can I do to help facilitate these changes? I'm a bit new to this process. Should I close the pull request?

Feel free to make another commit to this PR to change the code using the existing

from ..math import num_combinations

and add a note to the CHANGELOG.md file at ./docs/sources/CHANGELOG.md (in the bug fixes section).

Since GitHub now has this convenient "Squash and Merge" button, we don't have to worry about "too many" commits since that's all being taken care of at the end.

[admercs]

Hi @rabst, I did some testing and it appears that the results of the num_combinations function may be imprecise or incorrect? I tested it against the scipy.misc.comb function with exact=True for N=300, k=10 and found different outputs. The proposed nCr function produces identical results to scipy.misc.comb. You might want to verify this.

[rasbt]

Thanks for looking into that, @adam-erickson . I also find the same issue, when I

>>> num_combinations(n=200, k=10)
22451004309013280

>>> scipy.misc.comb(n=200, k=10)
22451004309013280

But for n=300, I get 1398320233241701888 & 1398320233241701770 (the later from scipy). I ran the same thing in Python 2.7 and noticed that Py 2.7 gives the correct results for num_combinations. Turns out to be an integer division problem. For instance, it should be

comb = numerator//denominator

instead of

comb = int(numerator/denominator)

Will fix that

[rasbt]

Should be fixed now in #200 (but you could also use scipy.misc (didn't know comb existed back then)

[rasbt]

Sorry, somehow, I forgot about this ... Merging now because the code is good and works perfectly fine :). Thanks for the contribution!