Publish reproducible PharmaPy documentation#131
Conversation
|
Review requested from the full current team. GitHub has formally notified the active non-author members. @ZacharyHillman18 @Piyush1698 @QHuckaba — your PharmaPy-org and repository invitations are still pending, so GitHub cannot add you as formal requested reviewers yet. Please accept those invitations and review this PR; after acceptance, you can also be added to the formal reviewer list. |
bernalde
left a comment
There was a problem hiding this comment.
Maintainer review of the documentation-publishing PR (17 files: docs infra, Sphinx config, [docs] extra, RTD/CI workflows, attribution, metadata). I authored this PR, so it is posted as a COMMENT — GitHub does not accept an author APPROVE/REQUEST_CHANGES. No blocking findings.
What I verified
- File set matches
gh pr diff(17 files); merge-base diff confirmed. - Python changes are docstring-only and accurate. Verified against the live signatures at head:
RxnKinetics: rename tostoich_matrixmatches the real parameter (Kinetics.py:214); the old docstring'sstoiciometric_matrix/stoichiometric_matrixspellings were wrong.BatchReactor.solve_unit: removedtimesim_limit(not a parameter — confirmed absent) and documented the realparams_control/sundials_opts; converted to NumPy-styleParameters/Returns.Evaporator/ContinuousEvaporator:**fig_kwargs→fig_kwargsis a numpydoc rendering fix; signatures unchanged.
- Assets referenced by
conf.pyare tracked (images/PharmaPy_logo.jpeg,images/purdue_logo.png,images/fda_logo.png,references.bib), so the broad.gitignore*.png/*.jpgrule does not strand them (git ignore does not affect already-tracked files). - Workflow
docs.ymluses least-privilegepermissions: contents: read,concurrencywithcancel-in-progress, builds tosite/(which the PR adds to.gitignore), and does not deploy — consistent with RTD owning publication. No generated HTML committed. .readthedocs.yamlinstalls.[docs],fail_on_warning: truematches the CI-Wbuild.- Attribution (
doc/online_docs/index.rst) preserves Original developers, adds Current developers and maintainers, and the joint-project acknowledgement (CryPTSys, SECQUOIA, Can Li, Eli Lilly) — matches #130's requirement. - Metadata URLs/maintainers moved to
PharmaPy-org.
Linked issue (#130)
Correctly Refs #130, not Closes — the RTD reconnection/live-site verification is an external admin step the PR intentionally leaves open. All repository-side acceptance criteria (docs extra, RTD config, warning-as-error CI, attribution, metadata/URLs, README badge, no committed HTML) are satisfied.
Nonblocking
- Incomplete
stoich_matrixrename in the same docstring. See inline. - README downloads badge still points to
CryPTSys/PharmaPywhile the CI badge and all URLs moved toPharmaPy-org. If historical download counts on the upstream repo are intended, fine; otherwise it is inconsistent with the rest of the metadata move. - Docs Python version differs between environments:
.readthedocs.yamlbuilds on3.10,.github/workflows/docs.ymlon3.11. Both build the same docs and CI is green; aligning them (or documenting the intent) would make the "reproducible build" claim tighter. .gitignoreimage foot-gun: the broad*.png/*.jpgignore required the manual!doc/online_docs/examples/img/pfd_flowsheet.pngexception, and every future committed doc image will need another. Consider un-ignoring the doc image directories (e.g.!doc/online_docs/**) instead of per-file exceptions. Not breaking today since existing assets are tracked.
Merge-readiness
No blocking findings. Live state at review time: state=OPEN, mergeStateStatus=CLEAN, reviewDecision empty. CI at head 41304fd: Core tests SUCCESS, Sphinx docs SUCCESS, Assimulo integration tests SUCCESS. Posting this review adds inline threads; since mergeStateStatus is CLEAN, required_conversation_resolution is not enforced, so they are not a merge gate. This author account cannot supply a formal APPROVE — GitHub currently reports no approval gate, but if branch protection requires one, another eligible maintainer must approve. Per #130, the issue should remain open until the external RTD connection and live-site check are done.
| ``stoich_matrix`` and ``partic_species`` have to be passed | ||
| (see below). The default is None. | ||
| stoiciometric_matrix : numpy array, optional | ||
| stoich_matrix : numpy array, optional |
There was a problem hiding this comment.
Nonblocking: this rename to stoich_matrix is correct (it matches the real __init__ parameter), but the pass is incomplete — the partic_species description a few lines below still refers to the old name: "the order of the names in 'partic_species' is that of the columns of 'stoichiometric_matrix'." Since the parameter is now stoich_matrix, update that prose reference too. While here, the keq_params entry is still the autogenerated TYPE, optional / DESCRIPTION. placeholder — worth filling in given this docstring is already being cleaned up. Neither blocks merge.
There was a problem hiding this comment.
Addressed in 9129d81: the participating-species prose now references stoich_matrix, and keq_params is documented as the dimensionless per-reaction equilibrium constant at tref_hrxn that enables reverse rates. The warning-as-error Sphinx build and all current-head CI jobs pass.
|
Addressed the review feedback on head
|
Summary
docsdependency extra and install it from Read the Docs and GitHub ActionsPharmaPy-org/PharmaPyVerification
sphinx-build -E -a -W --keep-going -b html doc/online_docs /tmp/pharmapy-docs-131-siteactionlint .github/workflows/docs.yml .github/workflows/ci.ymlpython -m pip checkin a fresh Python 3.11 docs environment installed with.[docs]python -m compileall -q PharmaPypython -m pytest tests/ -m "not assimulo"(56 passed, 4 skipped, 6 deselected)Read the Docs follow-up
This PR makes the repository build-ready, but it intentionally does not close
the issue. After merge, the preferred publication path is a separate Read the
Docs project connected to
PharmaPy-org/PharmaPy, leaving the legacypharmapy.readthedocs.iosite untouched. Issue #130 remains open until thatexternal project is registered and its live site, push builds, and pull-request
previews are verified.
Refs #130