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

New NAGL docs take 2 #21

Merged
merged 66 commits into from
Mar 22, 2023
Merged

New NAGL docs take 2 #21

merged 66 commits into from
Mar 22, 2023

Conversation

Yoshanuikabundi
Copy link
Contributor

@Yoshanuikabundi Yoshanuikabundi commented Jan 25, 2023

This PR adds documentation. Replaces #12 with a branch that has a clean history of not touching the main codebase.

Changes made in this Pull Request:

  • Set up automatic API documentation
  • Write docs

PR Checklist

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

docs/index.md Outdated Show resolved Hide resolved
@Yoshanuikabundi
Copy link
Contributor Author

Yoshanuikabundi commented Mar 9, 2023

This is a reminder to me to migrate from save_checkpoint() and load_from_checkpoint() to save() and load() once they become available. Done!

docs/theory.md Outdated

In a molecular graph, atoms are nodes and bonds are edges. A molecular graph can store extra information in its nodes and edges; in a Lewis structure, this is things like an atom's element and lone pairs, or a bond's bond order or stereochemistry, but for a molecular graph they can be anything. Element and bond order are usually somewhere near the top of the list. Envisioning a molecule as a graph lets us apply computer science techniques designed for graphs to molecules.

Note that a particular molecular species may have more than one molecular graph topology that represents it. This most commonly happens with tautomers and resonance forms. NAGL operates on molecular graphs, and on the OpenFF [`Molecule`] class, and it doesn't try to be clever about whether two molecules are the same or not. If you want tautomers to produce the same charges, you may need to prepare your dataset accordingly.
Copy link
Collaborator

Choose a reason for hiding this comment

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

(Non-blocking, since the ResonanceEnumerator is not very well documented and hidden in utils) -- this might be a good place to eventually mention the ResonanceEnumerator which will enumerate all resonance forms of a molecule so that properties such as formal charges or hybridisation can be averaged.

@Yoshanuikabundi
Copy link
Contributor Author

Yoshanuikabundi commented Mar 15, 2023

OK I think this is ready for review! Have no idea what's going on with the failing test What failing test 👼 I'm going to open separate PRs with additions to docstrings so that they don't create conflicts that delay this getting merged.

Copy link
Collaborator

@lilyminium lilyminium left a comment

Choose a reason for hiding this comment

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

The docs look beautiful -- thanks so much for all the work you put into this! LGTM!

Could you please add yourself to the AUTHORS list too?

docs/dev.md Outdated Show resolved Hide resolved
@Yoshanuikabundi Yoshanuikabundi merged commit bd220c0 into main Mar 22, 2023
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