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

Rearrange demos to have fewer folders #1471

Merged
merged 55 commits into from May 5, 2020
Merged

Conversation

huonw
Copy link
Member

@huonw huonw commented May 4, 2020

Per #1428, having many subfolders makes the docs display ugly and harder to understand.

This also renames the demos to have a consistent and globally unique filename, like <algorithm>-<extras>-<task>. This makes it easy to find a demo when looking within a file browser (such as the left hand panel of Jupyter lab) because they're sorted by algorithm name. It also ensures that a bug report just mentioning a filename is enough to work out exactly which demo and what task they're doing.

This retains an existing placeholder for the GCN node classification demo, since we've linked to that in various places as a "get started with StellarGraph" demo. An alternative would be to just rely/hope that we update all those lines (#1482). The placeholder has the same name as the original, and so there's no renamed detected for demos/node-classification/gcn/gcn-cora-node-classification-example.ipynb to demos/node-classification/gcn-node-classification.ipynb. However, this is just a rename, with some of the internal links updated.

Docs view: https://stellargraph--1471.org.readthedocs.build/en/1471/demos/index.html#table-of-contents

See: #1428

@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 May 4, 2020

Code Climate has analyzed commit fbd9708 and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category Count
Security 2

View more on Code Climate.

@huonw huonw mentioned this pull request May 4, 2020
@huonw huonw marked this pull request as ready for review May 5, 2020 01:51
Copy link
Contributor

@timpitman timpitman left a comment

Choose a reason for hiding this comment

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

The duplicate GCN is well hidden; I'm happy to release with it there, then remove next release once we're confident all links are updated.

The arrangement looks really good now, well done!

@timpitman
Copy link
Contributor

Thinking about this more; could the duplicate GCN demo just be a symlink?

@huonw
Copy link
Member Author

huonw commented May 5, 2020

Thinking about this more; could the duplicate GCN demo just be a symlink?

That's a good idea, but I think it won't work, because format_notebooks will be expecting its runner badges to include the symlink path (but they'll be the path for the main notebook).

@huonw
Copy link
Member Author

huonw commented May 5, 2020

This now builds without any (new) warnings, and a link checker run against our docs works except for some external links and various links to https://stellargraph.readthedocs.io/en/stable/..., because we've not yet published a stable release with these new docs. I've tried to manually verify them as much as possible.

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.

The new output looks really nice and clean to me 👍 And good idea to keep the old gcn notebook for now

@huonw huonw merged commit 04992cb into develop May 5, 2020
@huonw huonw deleted the bugfix/1428-less-folders branch May 5, 2020 04:06
kjun9 added a commit that referenced this pull request May 5, 2020
The notebook names/paths were updated as part of #1471 so the ignore path has to be updated so that the hateful twitter interpretability notebook doesn't get included in our API docs.

See #1500
huonw added a commit that referenced this pull request May 6, 2020
This fixes the remaining sphinx warnings from a local `make html`:

```
.../stellargraph/mapper/padded_graph_generator.py:docstring of stellargraph.mapper.PaddedGraphGenerator.flow:4: WARNING: Inline interpreted text or phrase reference start-string without end-string.
.../stellargraph/layer/gcn_lstm.py:docstring of stellargraph.layer.FixedAdjacencyGraphConvolution.call:5: WARNING: Unexpected indentation.
.../docs/demos/basics/index.txt:18: WARNING: toctree glob pattern '*/index' didn't match any documents
.../docs/demos/calibration/index.txt:14: WARNING: toctree glob pattern '*/index' didn't match any documents
.../docs/demos/connector/index.txt:7: WARNING: toctree glob pattern './*' didn't match any documents
.../docs/demos/connector/neo4j/index.txt:25: WARNING: toctree glob pattern '*/index' didn't match any documents
.../docs/demos/embeddings/index.txt:64: WARNING: toctree glob pattern '*/index' didn't match any documents
.../docs/demos/ensembles/index.txt:11: WARNING: toctree glob pattern '*/index' didn't match any documents
.../docs/demos/graph-classification/index.txt:34: WARNING: toctree glob pattern '*/index' didn't match any documents
.../docs/demos/index.txt:303: WARNING: toctree glob pattern './*' didn't match any documents
.../docs/demos/interpretability/index.txt:30: WARNING: toctree glob pattern '*/index' didn't match any documents
.../docs/demos/link-prediction/index.txt:69: WARNING: toctree glob pattern '*/index' didn't match any documents
.../docs/demos/node-classification/gcn-node-classification.nblink:556: WARNING: image file not readable: demos/node-classification/Cora-features.png
.../docs/demos/node-classification/index.txt:128: WARNING: toctree glob pattern '*/index' didn't match any documents
```

As well as an extra warning only visible on Read the Docs
https://readthedocs.org/projects/stellargraph/builds/10977970/ (not sure why it
doesn't appear locally):

```
WARNING: missing attribute mentioned in :members: or __all__: module stellargraph, attribute
```

There's four "categories":

- Invalid syntax: in `PaddedGraphGenerator.flow`, the `s` immediately after
  ":class:`StellarGraph`s" causes problems; in
  `FixedAdjacencyGraphConvolution.call`, the indentation is confusing Sphinx
- Missing file: the image of features in Cora `Cora-features.png` (used in the
  `gcn-node-classification` demo) was forgotten and thus disappeared in #1471,
  this adds it back.
- Globs without any files: there's various globs in the `index.txt` that were
  added mechanically; the mechanical adding mean they were as generic as
  required, handling both subfolders (`*/index`) and adjacent notebooks
  (`./*`). Now that we're maintaining RtD as the main source of notebooks, we
  can maintain these globs by hand.
- Trailing comma: the trailing comma in the `:members:` declaration causes it to
  be looking for the attribute after that comma (i.e. the empty string)


The RtD build seems to be similarly warning-free:
https://readthedocs.org/projects/stellargraph/builds/10978073/

See: #1360
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