Skip to content
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

Remove NodeSplitter and train_val_test_split #887

Merged
merged 3 commits into from Feb 18, 2020

Conversation

huonw
Copy link
Member

@huonw huonw commented Feb 18, 2020

The NodeSplitter class and the train_val_test_split function seemingly have only been used in their own tests for quite a while now, specifically:

  • 7f5cc7c from August 2018 seems to be the commit that removes the last use of NodeSplitter (by deleting demos/StellarGraph_examples.ipynb),
  • 611d3f2 from September 2018 seems to be the commit that removes the last use of train_val_test_split (by deleting demos/node-classification-hinsage/generalization-tests.py).

Given these aren't used in our examples, it seems like we can probably remove them, and reduce how much code we need to maintain.

Replacement: manually cutting up a pandas data frame, or using sklearn.model_selection.train_test_split (most of our examples do the latter, now).

See: #885

@codeclimate
Copy link

codeclimate bot commented Feb 18, 2020

Code Climate has analyzed commit d3d38ee and detected 0 issues on this pull request.

View more on Code Climate.

@codecov-io
Copy link

codecov-io commented Feb 18, 2020

Codecov Report

Merging #887 into develop will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           develop    #887   +/-   ##
=======================================
  Coverage     85.1%   85.1%           
=======================================
  Files           49      49           
  Lines         5018    5018           
=======================================
  Hits          4268    4268           
  Misses         750     750           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 97c188c...d3d38ee. Read the comment docs.

@huonw huonw marked this pull request as ready for review February 18, 2020 02:17
Copy link
Contributor

@kjun9 kjun9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@huonw huonw merged commit 9454a01 into develop Feb 18, 2020
@huonw huonw deleted the feature/885-remove-node-splitter branch February 18, 2020 23:33
@huonw huonw mentioned this pull request Feb 25, 2020
3 tasks
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.

None yet

4 participants