API docs updates #493

Yoshanuikabundi · 2023-07-19T02:02:22Z

This PR:

Adds an improved stylesheet for the API reference
Fixes the root cause necessitating workarounds in equil_rfe_methods.py to document protocol settings (important modules were mocked when they were present in the RTD environment)
Organises the API reference with Autosummary for easier browsing
Adds more member documentation to the API docs
Sets up Intersphinx for linking to other documentation
Fixes some typos and mistakes in the installation and user guide docs

There's also one possibly controversial change:

Annotate RelativeHybridTopologyProtocolSettings.forcefield_settings: OpenMMSystemGeneratorFFSettings. Previously it inherited the Settings.forcefield_settings: BaseForceFieldSettings annotation. This is a Pydantic model so this annotation is a runtime behavior change.

@dwhswenson and I briefly discussed this today. It seems like it may just be an oversight; RelativeHybridTopologyProtocol certainly assumes that it will be OpenMMSystemGeneratorFFSettings and has no support for other classes, but maybe downstream consumers are re-using the settings class for their own protocols. The change makes it much easier to understand and find what's supposed to go where. I'm happy to revert this for now though and sort it out when I kick up a stink about the 3 different default RelativeHybridTopologyProtocolSettings values 😅

There are new Sphinx warnings, but they relate to docstrings in GUFE that I'll submit a PR to fix in a moment - I'm pretty confident I know what's going on but if I'm wrong I can fix it here too.

Developers certificate of origin

I certify that this contribution is covered by the MIT License here and the Developer Certificate of Origin at https://developercertificate.org/.

codecov · 2023-07-19T02:16:31Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01% 🎉

Comparison is base (2d0fdad) 91.72% compared to head (8ffdccf) 91.73%.
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #493      +/-   ##
==========================================
+ Coverage   91.72%   91.73%   +0.01%     
==========================================
  Files         110      110              
  Lines        6607     6617      +10     
==========================================
+ Hits         6060     6070      +10     
  Misses        547      547

Files Changed	Coverage Δ
openfe/protocols/openmm_rfe/equil_rfe_settings.py	`87.44% <100.00%> (+0.61%)`	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Yoshanuikabundi · 2023-07-19T02:41:34Z

RTD is failing because it is configured not to permit warnings; the warnings introduced by this PR are fixed in OpenFreeEnergy/gufe#210

docs/_ext/sass.py

richardjgowers · 2023-07-19T07:59:41Z

docs/conf.py

 ]

-autoclass_content = 'both'
+intersphinx_mapping = {
+    "python": ("https://docs.python.org/3.7", None),


this should probably be our lowest supported version right? which iirc is 3.9 or 3.10?

Do we follow NEP29 (in which case 3.9)?

The installation guide just says 3.9 and 3.10 are tested so I'll update this to 3.9.

openfe/protocols/openmm_rfe/equil_rfe_settings.py

Co-authored-by: Richard Gowers <[email protected]>

dwhswenson

Looks good; some major improvements on what we had!

I had a couple of questions, but nothing that should block merging.

dwhswenson · 2023-07-25T21:30:21Z

docs/_ext/sass.py

+Modified to:
+- Write directly to Sphinx output directory
+- Infer targets if not given
+- Ensure ``target: Path`` in ``configure_path()``
+- Return version number and thread safety from ``setup()``
+- Use compressed style by default
+- More complete type checking
+"""


Is there a reason not to open a PR upstream for these changes? I'm fine with vendoring for now, but if upstream will take the changes, I'd rather be able to remove this from our maintenance burden.

The crucial change is that it now writes to the build directory rather than the source directory, which is helpful for minimising the mess that Sphinx re-builds can get into. Once I vendored it to fix that, it seemed worthwhile to make more changes to make it a more natural fit. I'll open a PR upstream and see what happens :)

dwhswenson · 2023-07-25T21:39:01Z

docs/guide/introduction.rst

@@ -34,7 +34,7 @@ The output of **setup** is an :class:`.AlchemicalNetwork`. This contains all
 the information about what is being simulated (e.g., what ligands, host proteins, solvation details etc) and the
 information about how to perform the simulation (the Protocol).

-The output of the **executation** stage is the basic results from each edge.
+The output of the **execution** stage is the basic results from each edge.


😂 This had to be me writing, but why was I repeatedly typing "executation"? Thanks for the typo fixes!

I was like, "is this a word with a slightly different meaning that no-one understands the distinction of", like dynamic vs dynamical or alchemic vs alchemical 😆

dwhswenson · 2023-07-25T21:49:24Z

openfe/protocols/openmm_rfe/equil_rfe_settings.py

+    # Inherited things
+
+    forcefield_settings: OpenMMSystemGeneratorFFSettings
+    """Parameters to set up the force field with OpenMM Force Fields."""
+    thermo_settings: ThermoSettings
+    """Settings for thermodynamic parameters."""
+


I take it there's no reasonable way to inherit these docstrings from parent classes? (Curious why, for my own understanding.)

For forcefield_settings, the base class is meant to be generic over other force field generators. For thermo_settings, it could be placed in the base class with a PR to GUFE.

IAlibay

it's late, feel free to accept / reject as needed.

IAlibay · 2023-07-25T23:56:20Z

openfe/protocols/openmm_rfe/equil_rfe_settings.py

@@ -149,7 +55,6 @@ def is_positive_distance(cls, v):
            raise ValueError(errmsg)
        return v

-


Shouldn't this be two empty lines for PEP8?

This is what happens when I can't use Black :(

I, with all my Canadian given authority, give you the unreserved power to apply black to that entire file 🙃

Sounds like a can of worms I probably shouldn't crack open just yet 😆

IAlibay · 2023-07-25T23:59:27Z

openfe/protocols/openmm_rfe/equil_rfe_settings.py

+    # Inherited things
+
+    forcefield_settings: OpenMMSystemGeneratorFFSettings
+    """Parameters to set up the force field with OpenMM Force Fields."""


Whilst this is true, I do worry this (talking about OpenMM Force Fields) might confuse the less experienced users? The intent originally was that this should just be "force field choices for how your system will be parameterized".

Discussed in today's call - there's probably nothing we can do about this for v1.0.

openfe/protocols/openmm_rfe/equil_rfe_settings.py

IAlibay · 2023-07-26T00:28:13Z

openfe/protocols/openmm_rfe/equil_rfe_settings.py

    alchemical_sampler_settings: AlchemicalSamplerSettings
+    """Settings for sampling within lambda windows."""


Suggested change

"""Settings for sampling within lambda windows."""

"""

Settings for alchemical space sampling, including the number of lambda windows

and simultaneous sampling replicas.

"""

Something like that, simultaneous sampling replicas is quite a mouthful so feel free to reject. P.S. forgive the formatting it's late.

The number of lambda windows is in AlchemicalSettings, and alchemical space sampling sounds to me like it would be searching through a space that includes different alchemical states. AlchemicalSamplerSettings seems focused on how to sample each window - could you clarify why my phrasing is incorrect/inadequate?

re: number of lambda windows - you're 100% right, I'm muddling myself with n_replicas.

re: why I think we need to maybe reword - beyond "sampling within lambda space", these settings also tackle things like the online analysis interval, and (unfortunately) the number of whole repeats of the entire simulation. I was trying (and probably failing to) reword it as something that covers that wider space.

Gotcha, I misunderstood how the protocol worked.

IAlibay · 2023-07-26T00:30:25Z

openfe/protocols/openmm_rfe/equil_rfe_settings.py

+    SettingsBaseModel,
+    OpenMMSystemGeneratorFFSettings,
+    ThermoSettings,
+)



Suggested change

Le PEP8?

Co-authored-by: Irfan Alibay <[email protected]>

Yoshanuikabundi · 2023-07-27T03:25:09Z

Am I good to merge this?

Yoshanuikabundi added 10 commits July 12, 2023 21:17

Add custom stylesheet for API reference

e307633

Improve code readability in dark mode

c4b8220

Typos

fddec84

Correct misleading time estimate

ffebeee

Add function autoproperty to CSS

42eea9c

Set up intersphinx

c883a05

Autosummary for API reference

f79e535

Fix openmm_rfe settings docs

4ff5ba2

Check in SASS rather than compiled CSS

eb2c67a

Merge branch 'main' into docs-updates

474fb35

Yoshanuikabundi mentioned this pull request Jul 19, 2023

Transformation docstrings fixes OpenFreeEnergy/gufe#210

Merged

richardjgowers reviewed Jul 19, 2023

View reviewed changes

Yoshanuikabundi and others added 6 commits July 20, 2023 14:27

Apply suggestions from code review

a60f9b6

Co-authored-by: Richard Gowers <[email protected]>

More consistent and contrasting colors

cb00685

Hide misleading field list from settings docs

6c49437

Limit reference TOC depth

c0d747f

Fix links in settings docs

096b6e2

Merge branch 'main' into docs-updates

17e7502

dwhswenson approved these changes Jul 25, 2023

View reviewed changes

IAlibay reviewed Jul 26, 2023

View reviewed changes

Yoshanuikabundi and others added 5 commits July 26, 2023 11:04

Update openfe/protocols/openmm_rfe/equil_rfe_settings.py

5517a27

Co-authored-by: Irfan Alibay <[email protected]>

PEP8

c0e76ff

Fix field ordering

3d5e4a0

Fix docstrings for protocol settings

5267ab8

Merge branch 'main' into docs-updates

8ffdccf

mikemhenry requested review from richardjgowers and IAlibay July 28, 2023 21:56

mikemhenry requested a review from dwhswenson July 28, 2023 21:56

IAlibay merged commit b513ded into main Jul 31, 2023
6 checks passed

IAlibay deleted the docs-updates branch July 31, 2023 15:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API docs updates #493

API docs updates #493

Yoshanuikabundi commented Jul 19, 2023

codecov bot commented Jul 19, 2023 •

edited

Loading

Yoshanuikabundi commented Jul 19, 2023

richardjgowers Jul 19, 2023

Yoshanuikabundi Jul 20, 2023

Yoshanuikabundi Jul 20, 2023

dwhswenson left a comment

dwhswenson Jul 25, 2023

Yoshanuikabundi Jul 26, 2023

dwhswenson Jul 25, 2023

Yoshanuikabundi Jul 26, 2023

dwhswenson Jul 25, 2023

Yoshanuikabundi Jul 26, 2023

IAlibay left a comment

IAlibay Jul 25, 2023

Yoshanuikabundi Jul 26, 2023

IAlibay Jul 26, 2023

Yoshanuikabundi Jul 27, 2023

IAlibay Jul 25, 2023

IAlibay Jul 26, 2023

IAlibay Jul 26, 2023

Yoshanuikabundi Jul 26, 2023

IAlibay Jul 26, 2023

Yoshanuikabundi Jul 27, 2023

IAlibay Jul 26, 2023

Yoshanuikabundi commented Jul 27, 2023

		@@ -149,7 +55,6 @@ def is_positive_distance(cls, v):
		raise ValueError(errmsg)
		return v

		alchemical_sampler_settings: AlchemicalSamplerSettings
		"""Settings for sampling within lambda windows."""

-    """Settings for sampling within lambda windows."""
+    """
+    Settings for alchemical space sampling, including the number of lambda windows
+    and simultaneous sampling replicas.
+    """

API docs updates #493

API docs updates #493

Conversation

Yoshanuikabundi commented Jul 19, 2023

Developers certificate of origin

codecov bot commented Jul 19, 2023 • edited Loading

Codecov Report

Yoshanuikabundi commented Jul 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwhswenson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IAlibay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yoshanuikabundi commented Jul 27, 2023

codecov bot commented Jul 19, 2023 •

edited

Loading