MobleyLab / chemper

Repository for Chemical Perception Sampling Tools
MIT License
19 stars 10 forks source link

Update Clusters to use tuples and update relevant documentation and examples #41

Closed bannanc closed 5 years ago

bannanc commented 5 years ago

The initial plan with this PR was to switch ClusterGraph to use tuples instead of dictionaries, but I ended up doing three things:

1. Switch to tuples #37

ClusterGraph used to expect dictionaries in the form {smirks index: atom index}. Now it will use tuples of atom indices instead where internally the SMIRKS indices are chosen based on the order of atom indices in the tuple.

That is a bond would be specified with two atoms (0,1) those atoms would then get SMIRKS indices :1 and :2 since there are two items in the tuple.

2. Update Jupyter notebooks and Documentation API/Examples #25

Since I needed to fix ALL the code that used ChemPerGraphs or ClusterGraphs, I decided to also do some updating on the language around these examples in the documentation and double checked that the Jupyter notebooks were more clear.

This means that PR #26 will focus on the "bigger picture" parts of the documentation webpage. Introductions, installation, and explanations on ChemPer is needed and what it does.

3. Reorder smirksify

This is not in an issue because it occurred to me while updating the webpage stuff. I had smirksify in a folder called optimizing_smirks. This doesn't really make sense since SMIRKSify is pretty much the main function for ChemPer. I instead moved smirksify.py up into the chemper folder and then moved environment.py into the graphs folder, after all it is a graph based class too.

codecov[bot] commented 5 years ago

Codecov Report

Merging #41 into master will increase coverage by 0.8%. The diff coverage is 86.95%.

vtlim commented 5 years ago

Here are my comments on the docs:

API

bannanc commented 5 years ago

I should have made this more clear, but the only parts of the website being updated in this PR were the API because it was affected by the code changes.

I don't want to end up with too many conflicts with PR #26 which is focused on updating the intro part of the documentation. The conda in the install is also fixed there. So I will move the checklist for the intro and installing over there. Though, the intro is being changed completely. Also in PR #26 I will make sure the code styling is consistent if you use a single accept symbol it is italicized and two accent symbols gets the color change into the "coding" format. I acknowledge there are still inconsistencies here, but I will make them all one or two accents in PR #26.

bannanc commented 5 years ago

Regarding this discussion around the order of inputs.

The thought process was that input in the form [ (mol, [list of tuples] ), ... ] would be easier for the user. However, the form for smirksify is [molecules] [ ('label', [ [ tuples], ... ]) ]. I don't want to make the user provide separate molecule objects for EVERY label! So actually think keeping clusters separate makes the most sense for now.

I am going to update the doc strings so there is always an example with multiple sets of tuples in one of the molecules.