[WIP] Rose

[andrealorenzon]

Reference Issue

First PR for implementation of Random OverSampling Example (ROSE) method.

What does this implement/fix? Explain your changes.

I drafted the class ROSE, but I have still some error on common test units. I would need a help to understand what I did miss.

Any other comments?

[pep8speaks]

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

• In the file imblearn/over_sampling/_rose.py:

Line 117:17: E123 closing bracket does not match indentation of opening bracket's line

Comment last updated at 2020-09-16 09:03:36 UTC

[lgtm-com]

This pull request introduces 4 alerts when merging 0a3307b into 0acd717 - view on LGTM.com

new alerts:

• 3 for Unused import
• 1 for Unused local variable

[hayesall]

I have still some error on common test units

It looks like most the errors raised are linting errors. The pep8speaks/lgtm entries above should have most of them. If you want to test things locally, running pylint on your files should help catch most of these!

The current version also looks like it''s pointing the user guide. It would be good to include a couple examples / an overview in the user guide as well.

[andrealorenzon]

I have still some error on common test units

It looks like most the errors raised are linting errors. The pep8speaks/lgtm entries above should have most of them. If you want to test things locally, running pylint on your files should help catch most of these!

The current version also looks like it''s pointing the user guide. It would be good to include a couple examples / an overview in the user guide as well.

Thank you. I'll fix linting, and add documentation in the user guide.
When I'm done, should I just make another PR?

[hayesall]

When I'm done, should I just make another PR?

I marked this as a draft while a couple of these other points get worked on. When you're ready for some feedback, mark it as "Ready for Review" and we'll iterate!

[andrealorenzon]

Are the PR automatically updated for new pushes on my fork repo?

[chkoar]

All pushed commits to andrealorenzon:ROSE will update the current PR.

[hayesall]

Still a few linting errors (see logs here), I missed mentioning flake8 earlier.

See the review comments otherwise.

[hayesall]

These are great to include. References should be referenced from the main docstring for the class. A short summary of the method would also be good.

Here's an example from the BorderlineSMOTE class:

imbalanced-learn/imblearn/over_sampling/_smote.py

Lines 219 to 224 in 0acd717

	class BorderlineSMOTE(BaseSMOTE):
	"""Over-sampling using Borderline SMOTE.
	This algorithm is a variant of the original SMOTE algorithm proposed in
	[2]_. Borderline samples will be detected and used to generate new
	synthetic samples.

[andrealorenzon]

I added a better description in the docstring.

I would like to add more complete information on the maths in the docs too, but I'm not familiar with Sphinx. Could you point me to some instructions or metadocumentation?

[hayesall]

Math typesetting should look familiar if you've seen LaTeX before, here's a short guide from sphinx's docs: https://www.sphinx-doc.org/en/1.0/ext/math.html. Syntax is based on reStructuredText, which feels similar to markdown but has a powerful directive system.

Could you point me to some instructions or metadocumentation?

Nothing specific to imblearn. If you want to learn more, sphinx's "Getting Started" guide is a good place to start. (I'd recommend it regardless. Sphinx is used for a huge number of projects, so the skill is extremely transferable).

Our Makefile and conf.py in the docs/ directory are fairly standard. Building local documentation then looks like:

cd docs/ make html xdg-open _build/html/index.html

[andrealorenzon]

Ok. I will just have some problem with plots, but I'll manage to add everything to the docs.

I have another issue: failing checks, see below. It's not very clear to me what they do address at, and what to fix.

[lgtm-com]

This pull request introduces 1 alert when merging 9ac2797 into 0acd717 - view on LGTM.com

new alerts:

• 1 for Unused import

[lgtm-com]

This pull request introduces 1 alert when merging c391ec3 into 0acd717 - view on LGTM.com

new alerts:

• 1 for Unused import

[andrealorenzon]

going back to local development, I don't want to spam broken builds