Improve docstrings as needed for OpenFE #220
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #220 +/- ##
=======================================
Coverage 99.12% 99.12%
=======================================
Files 36 36
Lines 1827 1827
=======================================
Hits 1811 1811
Misses 16 16
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Mostly looks good to me. One section title that I think will be very misleading, and so needs to be renamed, and a tiny fix that is because @Yoshanuikabundi fixed half of someone else's mistake, otherwise it would have gone unnoticed.
Co-authored-by: David W.H. Swenson <dwhs@hyperblazer.net>
|
I think this is ready to go! |
gufe/components/proteincomponent.py
Outdated
| @@ -56,7 +56,8 @@ | |||
|
|
|||
|
|
|||
| class ProteinComponent(ExplicitMoleculeComponent): | |||
| """Wrapper around a Protein representation. | |||
| """ | |||
| :class:`Component` representing a protein. | |||
There was a problem hiding this comment.
We really should rename this component - in reality this is just "a wrapper around something you placed in a PDB".
Not sure if we at least want to fix this in the docs?
There was a problem hiding this comment.
Small change recommended: a couple of times we talk about the DAG dependency order; we should be very explicit in which order that is.
I think it is in the order that follows the arrow of time, is that correct, @dotsdl @richardjgowers? In the sense that a "parent" node will come before a "child" node, i.e., that this order is a possible execution order for the DAG.
I know @Yoshanuikabundi is mainly moving existing text around, but as long as we're taking a closer look, let's clarify things.
| @@ -181,10 +152,18 @@ def _from_dict(cls, dct: dict): | |||
|
|
|||
| @property | |||
| def result_graph(self) -> nx.DiGraph: | |||
| """ | |||
| DAG of `ProtocolUnitResult` nodes with edges denoting dependencies. | |||
There was a problem hiding this comment.
Can we clarify here the direction of dependencies? (I honestly don't remember if edges point to "previous" or to "next")
There was a problem hiding this comment.
edges point to previous
| """ | ||
| `ProtocolUnitResult`s for each `ProtocolUnit` used to compute this object. | ||
|
|
||
| Results are given in DAG-dependency order. |
There was a problem hiding this comment.
Again, better to clarify exactly what order is meant here.
|
@dwhswenson I completely agree that these docstrings are unclear, but can we leave it for another PR? It'll need input from someone who knows what is going on and this PR is currently holding up OpenFreeEnergy/openfe#527. I've raised an #223 as a reminder. |
|
Yep... with #223 to remind, I think this is good. Sorry if this slows you down at all. I'm trying to take advantage of a new set of eyes on the docs as a chance to improve aspects that are unclear when we look at them with our own fresh eyes. |
This PR moves more docstrings out of
attributesections of docstrings and onto the corresponding object itself. This helps comprehensive API documentation in OpenFE. Required by OpenFreeEnergy/openfe#527