Update BalancedBinningReference quick method #1034
Conversation
tests/test_target/test_binning.py
Outdated
| def test_quick_method(self): | ||
| """ | ||
| Test the quick method with producing a valid visualization | ||
| """ | ||
| data = load_occupancy(return_dataset=True) | ||
| _, y = data.to_pandas() | ||
|
|
||
| visualizer = balanced_binning_reference(y, show=False) | ||
|
|
||
| assert isinstance(visualizer, BalancedBinningReference) | ||
| self.assert_images_similar(visualizer, tol=0.5) |
There was a problem hiding this comment.
@Kautumn06 @wagner2010 I notice that the Appveyor Miniconda test is hanging indefinitely on this test. One thing that you could try is either
- changing line 70 to
_, y = data.to_numpy()
OR - adding the
@pytest.mark.skipif(pd is None, reason="pandas is required")decorator to thetest_quick_methodfunction (since if you have the line_, y = data.to_pandas(), then the test requires Pandas, which not all testing environments will have)
There was a problem hiding this comment.
GM @rebeccabilbro @Kautumn06 I'll go for option 2 using the in-line editor. Thank you both!
updated test_binning to accomodate non-pandas environments.
updated balanced_binning_reference hyperlink
There was a problem hiding this comment.
@Kautumn06 Nice work on the balanced_binning_reference QM! Thank you!
updating line 70 to "_, y = data.to_numpy()" to try to pass tests.
tests/test_target/test_binning.py
Outdated
| def test_quick_method(self): | ||
| """ | ||
| Test the quick method with producing a valid visualization | ||
| """ | ||
| data = load_occupancy(return_dataset=True) | ||
| _, y = data.to_numpy() | ||
|
|
||
| visualizer = balanced_binning_reference(y, show=False) | ||
|
|
||
| assert isinstance(visualizer, BalancedBinningReference) | ||
| self.assert_images_similar(visualizer, tol=0.5) |
There was a problem hiding this comment.
@wagner2010 @Kautumn06 my apologies if my previous comment caused confusion; I should have been more clear!
When you skip a test, the skip-if decorator needs to go right on top of the function definition in order to work, e.g.:
@pytest.mark.skipif(pd is None, reason="pandas is required")
def test_quick_method(self):
"""
Test the quick method with producing a valid visualization
"""
data = load_occupancy(return_dataset=True)
_, y = data.to_pandas()
visualizer = balanced_binning_reference(y, show=False)
assert isinstance(visualizer, BalancedBinningReference)
self.assert_images_similar(visualizer, tol=0.5)I like to think of decorators as sparkly tiaras that you put on top of the function or method's head so that they can say "do you even know who I am?" 👑 Then Python sees the tiara first and changes how it treats the function/method (in this case knowing to skip the whole test if there's no Pandas installed on that machine)
Maybe @bbengfort and I can do a clinic on testing sometime... In the meantime though, you can read more about Python decorators here). Thanks again for pushing ahead with this PR & review!
There was a problem hiding this comment.
Thanks for sharing the resource! It helps us improve!
|
@wagner2010 @Kautumn06 I just figured out what's making the tests hang; hold tight, about to push the fix |
|
|
||
| if show: | ||
| visualizer.show() | ||
| else: | ||
| visualizer.finalize() |
There was a problem hiding this comment.
I think this is what was missing! We have the show=False parameter set on @Kautumn06's quick method test (as it should be!), but because we hadn't yet added the show/finalize logic to the actual quick method yet, when we got to the Appeyor/Miniconda test, it was trying to finalize instead of show, causing the test to hang indefinitely. This should fix it 🤞 🙏 😅
As part of Issue #960
showparameter to thebalanced_binning_referencequick method