The docs describe the package as it was in July; bring them to what it is - #305
Conversation
…t is Four statements in the docs were false against the code. `usage.md` said the tangent solver covered every operator but `gilmore/mie-gruneisen`; the suite parametrises over all fourteen and that case passes at 7.5e-07. It said nothing in the package locates a fitted point for a candidate, twenty lines after documenting `fit_candidate`. It priced `STANDARD_MODELS` at 168,072 grid solves; the current 23 candidates cost 139,272. `accuracy.md` called `1e-9 / 1e-11` the default; the default is `1e-8 / 1e-10`. Two links were broken: `materials.md` pointed at a README heading that does not exist, and `upstream.md` had its "back to the README" footer in the middle of the file with two defects listed after it. The nine bullets there are now the eight the text counts, with the `radial = 6` complex-radii bullet folded into the Mie-Gruneisen root it repeats and `calc_omega_N` labelled as IMR-vanilla. The larger gap was coverage. Nine public modules -- design, measure, gain, discriminate, diagnose, assimilation, store, optimize, parallel -- had no user-facing documentation at all, and `RelaxingMaterial`, `CubicZener`, `QuadraticKelvinVoigt` and `max_radius_ratio` were exported without a mention. `usage.md` gains sections for each, with examples run against the code (the design-to-batch chain returns the weights and counts shown), and `materials.md` gains the missing laws. Every reduction stated there was checked: Zener against `QuadraticZener(stiffening=0)` and against `RelaxingMaterial(NeoHookean)`, `CubicZener(cubic=0)` against `QuadraticZener`, all at or below 6e-13. The API reference was also incomplete, in a way the README instruction hid. `pdoc pyimr` documents only the package page, because `__init__` sets `__all__`, so the CI step carried a hand-written module list -- which had drifted to seven modules out of nineteen. `tools/api_docs.py` derives the list by the package's own rule for public (no leading underscore, declares `__all__`); CI and the README both use it. The type gate now passes without the optional PyMC extra installed. The four diagnostics were `reportMissingImports` on the lazy imports in `_pymc()` and on the test that `importorskip`s them; the runtime already handles the absence, and the ignores are the static counterpart.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73a3c7af0f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| expected_gain(matrices[0], questions).per_question # nats, unweighted | ||
| measure = optimal_measure(np.array(matrices), criterion=gain_criterion(questions)) |
There was a problem hiding this comment.
In this example, matrices comes directly from design_information, which returns raw J.T @ J in unit-cube coordinates, while expected_gain and gain_criterion require prior-standardized information and compute their result from I + M. The package's own expected_information_gain applies the default uniform-prior variance of 1/12 before doing this calculation, but these two calls omit it; users following the example will therefore overstate gains and may optimize to a different batch. Scale these matrices by 1/12 (or sandwich them with the applicable prior standard deviations) before both calls.
Useful? React with 👍 / 👎.
| API reference: `pip install 'PyIMR[docs]'`, then `python tools/api_docs.py` | ||
| (every public module; `python -m pdoc pyimr` alone stops at the package page). |
There was a problem hiding this comment.
For a user following the preceding pip install 'PyIMR[docs]' instruction outside a source checkout, this command fails because tools/api_docs.py is not installed: setuptools discovery includes only pyimr*, and there is no console-script entry point. The new API-doc workflow is therefore usable by CI and repository contributors but not by the installed-package users addressed here; expose the helper from the package/an entry point or document a command that only uses installed files.
Useful? React with 👍 / 👎.
…appened to hit a zero pivot `separability` promised infinite variance for a model difference the material absorbs, and delivered it by catching `LinAlgError` from `np.linalg.inv`. The augmented Gram for such a difference is singular only to roundoff -- eigenvalues 1.2e-14, 97 and 162 on the test case -- and LAPACK raises only on an exactly zero pivot. OpenBLAS on x86 produced one, which is why the test passed on CI; Accelerate on arm64 produced 1e-14 and a variance of 2.8e14, which is the number the docstring warns invites a design that chases it. The null space is now found from the eigendecomposition at `matrix_rank`'s tolerance, and a rival coordinate with a component in it reports `inf`; the identified coordinates take the pseudo-inverse diagonal, which agrees with the old inverse to 4e-16 where that inverse existed. A zero-weight rival still gives an exact zero eigenvalue and `inf`, as before. Separately, the two standalone validation scripts under `tools/` imported `pyimr` with no path shim, so `python tools/validate_thermal_fd.py` from a checkout that was never `pip install -e`'d could not find the package, and the test asserting they still run failed for that reason alone. Both now insert the repository root first, as `benchmarks/run.py` already does.
…d ask `test_vanishing_correlation_time_reduces_to_independent_noise` asserted bitwise equality between two likelihoods computed by different arithmetic: the correlated path sums `n log 2pi + 2 sum log L_ii` and whitens by a triangular solve, the independent path sums `log(2 pi s^2)` and divides. They agree to an ulp, and which ulp moved between scipy 1.17 and 1.18, so the test passed on one and failed on the other. It now asserts agreement to 1e-12 relative.
1 participant
What was wrong
Four statements contradicted the code:
usage.mdsaid sensitivities cover every operator butgilmore/mie-gruneisen. The suite parametrises over all fourteen; that case passes at rel 7.5e-07.usage.mdsaid nothing in the package locates a fitted point for a candidate.fit_candidateis documented twenty lines above.usage.mdpricedSTANDARD_MODELSat 168,072 grid solves atcount=12. The current 23 candidates cost 139,272.accuracy.mdcalled1e-9 / 1e-11the default tolerance. It is1e-8 / 1e-10.Two links were broken (
materials.mdto a README anchor that does not exist;upstream.mdwith its footer mid-file and two defects after it). The defect list now has the eight bullets the prose counts.What was missing
Nine public modules had no user-facing docs:
design,measure,gain,discriminate,diagnose,assimilation,store,optimize,parallel.RelaxingMaterial,CubicZener,QuadraticKelvinVoigtandmax_radius_ratiowere exported without a mention.usage.mdandmaterials.mdgain sections for each; the README's capabilities paragraph and materials list follow.Every example and reduction claim added was run against the code: the design-to-batch chain returns the weights and counts shown; Zener vs
QuadraticZener(stiffening=0), Zener vsRelaxingMaterial(NeoHookean), andCubicZener(cubic=0)vsQuadraticZeneragree at or below 6e-13.API reference
pdoc pyimrdocuments only the package page, because__init__sets__all__. The CI step carried a hand-written module list that had drifted to seven modules out of nineteen, and the README told users to run the command that stops at one page.tools/api_docs.pyderives the list (no leading underscore, declares__all__); CI and the README use it. Verified locally: 21 pages.Type gate
Pyright now passes without the optional PyMC extra installed: four
reportMissingImportson the lazy imports in_pymc()and theimportorskip'd test get targeted ignores, matching the runtime handling that was already there.Checked
ruff check .clean;tools/pyright_baseline.pyreports no new errors (0 baselined)separabilitydetected an absorbed model difference by whethernp.linalg.invraised, which depends on LAPACK hitting an exactly zero pivot (OpenBLAS/x86 did; Accelerate/arm64 gave2.8e14instead ofinf), and now finds the null space atmatrix_rank's tolerance; and the twotools/validate_*.pyscripts importedpyimrwithout the path shimbenchmarks/run.pyhas, so they could not run from an uninstalled checkout..venvandpip install -e ".[test,inference,docs]"(jax 0.11.1, scipy 1.18.1, pymc 6.3.2), the complete suite including the slow set: 1013 passed, 2 skipped; packaging and PyMC tests included; wheel builds; 21 API pages.test_vanishing_correlation_time_reduces_to_independent_noiseasserted bitwise equality between two arithmetic paths that agree to an ulp, and the ulp moved with scipy 1.18. It now asserts 1e-12 relative.