-
Notifications
You must be signed in to change notification settings - Fork 2
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
test: create unit tests for MapReduce #300
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bk958178 This tests the interface as I expect it. Good work! I've thought of one more easily testable part of the algorithm.
strategy = cr.MapReduce(num_points=10, leaf_size=20) | ||
coreset = cc.RandomSample() | ||
coreset.original_data = orig_data | ||
strategy.reduce(coreset) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add a check that the recursive call is working correctly please? MapReduce._reduce_recursive
should be called at least twice.
I'm sure there's some complicated mocking that could make the number of calls and what is sent to each call deterministic. If you can quickly work out how to do it, great; otherwise, don't worry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call adding this check.
I think we can do this with .call_count
. See my latest commit. Thanks in advance for re-reviewing!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the principle of what I wanted, but I think you need to patch _reduce_recursive
before you can do this. https://docs.python.org/3/library/unittest.mock.html#unittest.mock.patch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've patched the method. Thanks in advance for re-re-reviewing 🎾
strategy = cr.MapReduce(num_points=10, leaf_size=20) | ||
coreset = cc.RandomSample() | ||
coreset.original_data = orig_data | ||
strategy.reduce(coreset) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the principle of what I wanted, but I think you need to patch _reduce_recursive
before you can do this. https://docs.python.org/3/library/unittest.mock.html#unittest.mock.patch
This PR is closing as it is outdated. The up-to-date PR for testing MapReduce is #317 |
PR Type
Description
Unit tests for MapReduce() class, in ReductionStrategy() in reduction.py.
How Has This Been Tested?
TODO. Tests currently failing due to missing dependencies
Does this PR introduce a breaking change?
No
Screenshots
Checklist before requesting a review