Hello@MattEding! Thanks for updating this PR. We checked the lines you've touched forPEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-11-22 00:59:05 UTC

Thanks Matt! That would be a nice addition!

Rough draft for online documentation illustrations of how the SPIDER algorithm works for arbitrary X and y values. The generated plots work as planned; just need to add written text discussing how to interpret graphs. Also needs to be moved from .ipynb to .py that can be understood by sphinx.
https://github.com/MattEding/SPIDER-Algorithm/blob/master/SPIDER-Formulation.ipynb

Codecov Report

Merging#603 intomaster willdecrease coverage by0.02%.
The diff coverage is97.92%.

@@            Coverage Diff             @@##           master     #603      +/-   ##==========================================- Coverage   98.46%   98.43%   -0.03%==========================================  Files          82       86       +4       Lines        4886     5062     +176     ==========================================+ Hits         4811     4983     +172- Misses         75       79       +4

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last update3839df1...53dfc4e. Read thecomment docs.

With Travis CIDISTRIB="ubuntu", it appears that this build is using Tensorflow 2.0 as the unit tests are failing withAttributeError: module 'tensorflow' has no attribute 'placeholder'.

I would suggest moving this implementation intosmote_variants. The idea behind this move is to benchmark the smote variants on a common benchmark on a large number of datasets and include in imbalanced-learn only the versions that show an advantage. You can see the discussion and contribute to it:https://github.com/gykovacs/smote_variants/issues/14

@MattEding would this strategy would be fine with you?

I believe thatSPIDER would be great addition toimblearn package. I need to find some time to thoroughly review it.@MattEding thanks for the patience.

@glemaitre I understand the motivation for not wanting to include it until it has a more proven track record. My only reservation about trying to include it insmote_variants is that SPIDER is unrelated to SMOTE. One of its motivations was to avoid creating synthetic samples:

"Random introduction of synthetic examples by SMOTE may be questionable or difficult to justify in some domains, where it is important to preserve a link between original data and a constructed classifier (e.g., to justify suggested decisions)."
Stefanowski and Wilk, 2008.Selective Pre-processing of Imbalanced Data for Improving Classification Performance.

As a side note, you may want to revisitNew Methods inviting people to contribute new sampling techniques by specifying that contributions may need to go through other repositories first.

My only reservation about trying to include it in smote_variants is that SPIDER is unrelated to SMOTE.

This is a really could point, I misread when reviewing the different PR :).

My proposal was a bit too quick.@chkoar has some concerns also regarding my proposal as well. We probably need to open an issue to have a public discussion about it.

So as a summary, let's go-ahead for the inclusion of this method. I should have a bit of time to review it in the coming days.@MattEding could you solve the conflict?

This pull requestintroduces 4 alerts when merging504d601 into9b31677 -view on LGTM.com

new alerts:

• 2 for Unused local variable
• 1 for Unused import
• 1 for Syntax error

This pull requestintroduces 3 alerts when merging2517bd9 intof356284 -view on LGTM.com

new alerts:

• 2 for Unused local variable
• 1 for Unused import

This pull requestfixes 3 alerts when merginga9cd91c intof356284 -view on LGTM.com

fixed alerts:

• 2 for Mismatch in multiple assignment
• 1 for Wrong name for an argument in a call

This pull requestintroduces 1 alert when merginga8d21e0 into9a1191e -view on LGTM.com

new alerts:

• 1 for Wrong name for an argument in a class instantiation

I am unfamiliar with how codecov determines bad lines of code coverage. With try-except blocks being normal control flow in Python--in fact it's consideredgood practice I am curious as to why these are being flagged as poor coverage. For example, the catching of anUnboundLocalError is a lot cleaner than saylocals().get('discard_indices', []). Also I am presuming that the red flags in the helper functions are due to multiple return statements. Is this the case?

How do the things look like? It would be nice to have this algorithm

@nschlaffke
It's been quite some time since I've worked on SPIDER, but I believe that I have left it as a fully working algorithm--it passes the (very small) toy test data set that I worked out manually. See examples/combine/plot_illustration_spider.py and the unit tests for details.

The things that needed to be fleshed out would be how it integrates with the rest of imblearn, and whether to lower the Code Coverage threshold rejecting try-except clauses or rewrite in different style. For example, I had created a new base class for this since it didn't feel like it would fit the role of over/under sampling. This may not be what the maintainers would desire or may want it to be refactored.

I will try to have a look soon. I had to make some underground work to ensure the compatibility with scikit-learn 0.23 and I should be done with it.

I will check when I get time. We can put a milestone for 0.9.