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

Fixes to docs and an example #73

Merged
merged 4 commits into from
Mar 21, 2024
Merged

Fixes to docs and an example #73

merged 4 commits into from
Mar 21, 2024

Conversation

Yoshanuikabundi
Copy link
Contributor

@Yoshanuikabundi Yoshanuikabundi commented Dec 19, 2023

I found over at openff-docs that train-gnn-notebook.ipynb had broken at some point since 0.2.2. This fixes that. I think this'll let us bring the central examples up to date with nagl but I'm not sure. Then I found that the API docs were not very complete - they were failing to a bug in Sphinx 5, so I've upgraded to Sphinx 6 and MyST 1.0... which then opened a big can of worms.

Changes made in this Pull Request:

  • Add version argument to train-gnn-notebook.ipynb
  • Update Sphinx to 6.x
  • Update MyST to 1.0
  • Switch to Conda Forge DGL in examples and docs environments
  • Update autosummary templates to work with Sphinx 6
  • Fix new Intersphinx link format for MyST 1.0 (it's now (<Link text>)[inv:<package>#<reference>] instead of (<Link text>)[<package>:<reference>])
  • Fix docstring warnings where possible (main one was that continuing lines in lists in RST must be indented to match the start of the text of the line, not the list signifier)

So it's:

1. this is a very long
   sentence
2. point two

not

1. this is a very long
sentence
2. point two

Questions:

  • Would you like me to set up a GitHub Action to run the examples?
  • There are some MyST warnings from theory.md and training.md because the relevant parts of the API seem to have been moved - how should they be updated?
/home/joshmitchell/Documents/openff/nagl/docs/theory.md:91: WARNING: 'myst' cross-reference target not found: 'openff.nagl.storage.record.MoleculeRecord.from_openff' [myst.xref_missing]
/home/joshmitchell/Documents/openff/nagl/docs/theory.md:91: WARNING: 'myst' cross-reference target not found: 'openff.nagl.storage.record.MoleculeRecord.from_precomputed_openff' [myst.xref_missing]
/home/joshmitchell/Documents/openff/nagl/docs/training.md:17: WARNING: 'myst' cross-reference target not found: 'openff.nagl.storage.record.MoleculeRecord' [myst.xref_missing]
/home/joshmitchell/Documents/openff/nagl/docs/training.md:55: WARNING: 'myst' cross-reference target not found: 'openff.nagl.nn.dataset.DGLMoleculeLightningDataModule' [myst.xref_missing]

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?

@codecov-commenter
Copy link

codecov-commenter commented Dec 19, 2023

Codecov Report

Merging #73 (e54a2ab) into main (64f9c25) will decrease coverage by 0.03%.
Report is 4 commits behind head on main.
The diff coverage is 100.00%.

Additional details and impacted files

Yoshanuikabundi added a commit to openforcefield/openff-docs that referenced this pull request Dec 19, 2023
Yoshanuikabundi added a commit to openforcefield/openff-docs that referenced this pull request Dec 19, 2023
@lilyminium
Copy link
Collaborator

Amazing, thank you @Yoshanuikabundi!

Would you like me to set up a GitHub Action to run the examples?

This would be great -- yes please!

There are some MyST warnings from theory.md and training.md because the relevant parts of the API seem to have been moved - how should they be updated?

The storage classes don't exist anymore as I've mostly moved towards just using plain Arrow datasets instead, which were more flexible when trying to work with multiple/custom targets. Looking through theory.md, those references could just be removed since for training a openff.nagl.molecule._dgl.molecule.DGLMolecule is just constructed from an OpenFF Molecule (for inference, a networkx-enabled openff.nagl.molecule._graph.molecule.GraphMolecule may be constructed instead).

For the training.md document I have code I'd like to adapt to replace that (and generally update the documentation) -- unfortunately I don't have that exactly to hand at the moment as most of it's on the lilac cluster, which I've temporarily lost access to for the moment. The DGLMoleculeLightningDataModule has generally been replaced by a configuration classes, specifically openff.nagl.config.data.DataConfig.

@Yoshanuikabundi
Copy link
Contributor Author

Mmm OK maybe we should leave further docs changes for a future PR then! I'll see if I can put together a quick examples runner to finish this one off.

"No... There is another" - Yoda
@lilyminium
Copy link
Collaborator

Thanks @Yoshanuikabundi! The changes all look good to me so far, let me know when you're after an official review :-)

@lilyminium
Copy link
Collaborator

@Yoshanuikabundi just picking this back up again, should I keep waiting to review in case you're planning any future changes or is this good to go? :-)

@lilyminium
Copy link
Collaborator

@Yoshanuikabundi I'll merge this for now so I can work off it for updating examples.

@lilyminium lilyminium merged commit 9c6258a into main Mar 21, 2024
40 checks passed
@lilyminium lilyminium deleted the examples_fixes branch March 21, 2024 07:24
@lilyminium
Copy link
Collaborator

lilyminium commented Mar 21, 2024

I think the changes in this PR fixed #92 too, hurray! Thank you very much @Yoshanuikabundi for the great work here.

@Yoshanuikabundi
Copy link
Contributor Author

Ah awesome! Sorry, I completely dropped this on the floor and forgot about it.

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.

3 participants