search

openforcefield / openff-toolkit

The Open Forcefield Toolkit provides implementations of the SMIRNOFF format, parameterization engine, and other tools. Documentation available at http://open-forcefield-toolkit.readthedocs.io
http://openforcefield.org
MIT License
302 stars 88 forks source link

Improve Toolkit Wrapper and Registry API docs #1787

Closed Yoshanuikabundi closed 2 months ago

Yoshanuikabundi commented 7 months ago

Updates some out-of-date docs tooling and fixes warnings so that the next time things break, we get a red check mark! Also adds NAGLToolkitWrapper to the docs and to the toolkits module, and breaks the Toolkit API reference out of the Utilities section so it can be discovered more easily.

codecov[bot] commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 93.49%. Comparing base (d8adf11) to head (cb2a449). Report is 5 commits behind head on main.

Additional details and impacted files
Yoshanuikabundi commented 7 months ago

I think remaining CI failures are fixed in #1786

j-wags commented 6 months ago

I haven't read everything yet - I am happy to back later to finish a review @j-wags

Yes @mattwthompson - Happy to have you take this one on. Please go ahead and review+merge when approved. Thanks!

Yoshanuikabundi commented 6 months ago

This is awesome work, the sort of stuff I wish was written years ago but didn't have the motivation to do myself.

I would love to take credit, but toolkits.md is all cut and pasted from utilities.rst - it's existing documentation that I've just moved to be a bit more discoverable. I can see that it needs some editing, but should we leave that for a future PR?

mattwthompson commented 6 months ago

Okay, so then the changes in this PR are

  1. Moving that file around as you describe
  2. Lots of (waves hands, ghostly noises appear out of the background, smoke machine revs up, lighting rains down from the sky) changes to make Sphinx/readthedocs happy
  3. Adding a docstring into the NAGL wrapper

Even though it's not part of what you originally wanted, I would like to see a couple of changes to that toolkits.md file. What's the lowest-friction way of moving this forward? I don't want to hang up the other changes on the basis of wanting to refresh existing docs, and it might be worth splitting this out into 2-3 separate PRs even though that incurs some cost.

Yoshanuikabundi commented 6 months ago

OK I've broken this PR up into:

  1. 1795, which should be simple to review

  2. 1796, which has a hellish change list but is mostly just converting ReST links to MarkDown links

  3. This PR, which now only breaks the Toolkit Wrapper docs into their own page (to preserve existing review comments)

I also rebased this PR on #1795 to avoid future merge conflicts, so #1795 needs to be merged before this one (because it's gonna be a while before I get to refreshing these docs). Accordingly, I've marked this as a draft. #1796 is now independent.

Fortunately I seem to have actually had my head on straight when I was making commits and this was actually very doable!

Yoshanuikabundi commented 2 months ago

Hey @mattwthompson, finally got around to this, I've implemented everything you suggested I think but have also moved stuff around to be less overwhelming at first glance. A lot more detailed info is in docstrings now. I also couldn't think of a way to skip only one type of error, so the example for that throws only one kind of error and skips the rest. That might have been what you meant anyway now that I think about it!