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 demo scripts that duplicate notebooks (#575) #889

Merged
merged 6 commits into from Feb 18, 2020

Conversation

timpitman
Copy link
Contributor

@timpitman timpitman commented Feb 18, 2020

Over time, StellarGraph has moved to Jupyter notebooks for demos, rather than scripts.

After some discussion, it appears that the demo scripts were generally used for development and debugging, and hence are no longer required. Removing them will remove the need to add them to CI for #575 and will also reduce our code surface for future refactoring, allowing us to concentrate on demo notebooks.

The only script remaining is: demos/node-classification/hinsage - this has no Jupyter notebook equivalent, and also can't be tested on CI due to use of the YELP dataset, and is listed with other untestable notebooks in #818.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

You'll be able to see Jupyter notebook diff and discuss changes. Powered by ReviewNB.

@codeclimate
Copy link

codeclimate bot commented Feb 18, 2020

Code Climate has analyzed commit d843e8c and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Security 1

View more on Code Climate.

@codecov-io
Copy link

codecov-io commented Feb 18, 2020

Codecov Report

Merging #889 into develop will decrease coverage by 0.6%.
The diff coverage is 51.7%.

Impacted file tree graph

@@            Coverage Diff            @@
##           develop    #889     +/-   ##
=========================================
- Coverage     85.7%   85.1%   -0.6%     
=========================================
  Files           45      49      +4     
  Lines         4901    5014    +113     
=========================================
+ Hits          4200    4266     +66     
- Misses         701     748     +47
Impacted Files Coverage Δ
stellargraph/core/element_data.py 92.2% <100%> (-0.3%) ⬇️
stellargraph/connector/__init__.py 100% <100%> (ø)
stellargraph/connector/neo4j/__init__.py 100% <100%> (ø)
stellargraph/data/loader.py 54.2% <100%> (+32.2%) ⬆️
stellargraph/connector/neo4j/mapper.py 33.9% <33.9%> (ø)
stellargraph/connector/neo4j/sampler.py 35.6% <35.6%> (ø)
stellargraph/layer/graphsage.py 79.4% <66.7%> (+0.1%) ⬆️
stellargraph/datasets/datasets.py 97.6% <96.3%> (-2.4%) ⬇️
... and 3 more

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 5ec46e6...d843e8c. Read the comment docs.

@timpitman timpitman marked this pull request as ready for review February 18, 2020 03:23
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 👍

As for the HinSAGE script, I think we should turn that into a notebook as well even if we can't run it in CI?

Copy link
Contributor

@PantelisElinas PantelisElinas left a comment

Choose a reason for hiding this comment

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

If that's what you want to do, then deleting the scripts is fine by me.

@timpitman
Copy link
Contributor Author

Looks good to me 👍

As for the HinSAGE script, I think we should turn that into a notebook as well even if we can't run it in CI?

#892

@timpitman timpitman changed the title Remove demo scripts that duplicate notebooks Remove demo scripts that duplicate notebooks (#575) Feb 18, 2020
@timpitman timpitman merged commit bb88314 into develop Feb 18, 2020
@timpitman timpitman deleted the feature/575-remove-demo-scripts branch February 18, 2020 05:53
timpitman added a commit that referenced this pull request Feb 25, 2020
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