Skip to content

Fix docstring for generate_data_clusters#646

Open
RealDream21 wants to merge 1 commit into
yzhao062:masterfrom
RealDream21:patch-1
Open

Fix docstring for generate_data_clusters#646
RealDream21 wants to merge 1 commit into
yzhao062:masterfrom
RealDream21:patch-1

Conversation

@RealDream21
Copy link
Copy Markdown

All Submissions Basics:

  • [V] Have you followed the guidelines in our Contributing document?
  • [V ] Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • [V] Have you checked all Issues to tie the PR to a specific one?

All Submissions Cores:

  • [V] Have you added an explanation of what your changes do and why you'd like us to include them?

I have stumbled across this while trying to use this function. The only thing I changed is the docstring to match the expected output ( in the line before the return there is a comment talking about the output but is different from what the docstring is saying).

@yzhao062
Copy link
Copy Markdown
Owner

Thanks for the fix. The docstring reorder is correct: generate_data_clusters returns train_test_split(X, y, ...), which yields (X_train, X_test, y_train, y_test), and the current Returns block has the wrong order.

Before I can merge, this PR needs to be rebased. The branch was opened off 3070497 (before commits d07f9c7 and bfbb3a6 landed), and as a result it lacks about 200 lines of code that the time-series detectors (generate_ts_data) and graph utilities (generate_graph_data) added in the meantime. The branch's copy of pyod/utils/data.py predates those additions, and GitHub currently reports the PR as conflicting. Rebasing avoids a conflict resolution that could accidentally drop generate_ts_data and generate_graph_data.

Could you rebase on the latest target branch (yzhao062/pyod:master, often upstream/master locally) and force-push? If this should land through development instead, please retarget the PR to development and rebase on yzhao062/pyod:development. After that I can rerun the review/checks and merge if everything is clean.

If you would rather not rebase, just let me know and I will land the docstring fix as a small cherry-pick with co-author credit so it is preserved.

Either way, thanks again for catching the mismatch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants