From 6fad858bc62ff0ada1610c688c1d02be2b63f853 Mon Sep 17 00:00:00 2001 From: rdk Date: Tue, 19 May 2026 15:36:12 +0200 Subject: [PATCH] Audit follow-ups: bug fix, doc refresh, exception taxonomy, test hardening MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug fix: - PrincipalMomentsDescriptor.clampNonNegative now also clamps NaN. The v<0 check was false for NaN, so a NaN eigenvalue (possible if a future code path bypasses GridGenerator.isFiniteBox) would have propagated to the CSV output. Doc refresh: - breaking-changes.md: 2.6 entry for the multi-column descriptor migration + the -vis_pocket_grid / pocket_grid_vis_* renames. - export-pocket-descriptors.md: step 4 rewrites a self-contradicting rationale — adding to the default list IS a breaking change for index-based parsers; recommends parse-by-name + breaking-changes.md note for future additions. - export-pocket-grid.md: added "Adding a new per-grid-point descriptor" recipe (parallel to the per-pocket one); unified √3/2 precision to 0.866 across docs and Params.groovy. - README.md: added an "Opt-in tabular exports" subsection mentioning -export_pocket_descriptors, -export_pocket_grid, -vis_pocket_grid. - testsets.sh "Full descriptor menu" now lists all seven shipped descriptors (was six). Exception taxonomy: - PocketDescriptorsRows.groovy and PocketGridBuilder.java now throw PrankException (was IllegalArgumentException) for user-facing config errors, matching the rest of the codebase. Registry hardening: - Both PocketDescriptorRegistry and PocketGridPointDescriptorRegistry now assert columnNames.size() == columnTypes.size() in register(). A future descriptor with mismatched lists fails fast at class-load. Quality fixes: - PocketGridRows.getColumn uses BASE_COLS-1 instead of literal 3 for the pocket column. Removed dead 2-arg PocketGridRows constructor (only 3 test sites used it; now inlined). - PocketGridPointContext gets a compact-constructor validator that rejects negative pointIndex/pocketRank, limiting blast radius of an int-arg swap. Test hardening: - VolsiteSmoothGridPointDescriptorTest + VolsiteGridPointDescriptorTest now pin sigma/radius in @BeforeEach AND restore in @AfterEach, so the Params singleton is clean for subsequent test classes. - New tests: HIS ND1 double-flag (single atom setting donor+acceptor), PrincipalMoments at cardinality=2, PrincipalMoments two coincident points, GridGenerator NaN-box throw, PocketDescriptorRegistry register/unregister round-trip, MorphologicalCloser maxIters=1. - Renamed respectsMaxIters → maxItersZeroIsNoOp (the test only covered the maxIters=0 case despite the general name); added maxIters=1 companion that verifies one iteration of fill actually runs. - Extracted RendererTestFixtures.tinyGrid (was byte-identical in both renderer test files); unified the volsite atomAt signatures so the parameter order can't get swapped between the two volsite tests. --- README.md | 4 ++ breaking-changes.md | 12 ++++ documentation/export-pocket-descriptors.md | 13 ++-- documentation/export-pocket-grid.md | 32 ++++++++- misc/test-scripts/testsets.sh | 4 +- .../output/PocketDescriptorsRows.groovy | 2 +- .../predict/output/PocketGridRows.groovy | 6 +- .../descriptors/PocketDescriptorRegistry.java | 6 ++ .../PrincipalMomentsDescriptor.java | 10 ++- .../output/grid/PocketGridBuilder.java | 2 +- .../descriptors/PocketGridPointContext.java | 13 ++++ .../PocketGridPointDescriptorRegistry.java | 6 ++ .../samplers/GridGeneratorBetweenTest.groovy | 18 +++++ .../predict/output/PocketGridRowsTest.groovy | 10 +-- .../descriptors/PocketDescriptorsTest.groovy | 69 +++++++++++++++++++ .../grid/MorphologicalCloserTest.groovy | 22 +++++- .../output/grid/PocketGridBuilderTest.groovy | 2 +- .../VolsiteGridPointDescriptorTest.groovy | 38 ++++++++++ ...olsiteSmoothGridPointDescriptorTest.groovy | 31 ++++++--- .../PocketGridChimeraXRendererTest.groovy | 18 +---- .../PocketGridPymolRendererTest.groovy | 20 +----- .../renderers/RendererTestFixtures.groovy | 46 +++++++++++++ 22 files changed, 317 insertions(+), 67 deletions(-) create mode 100644 src/test/groovy/cz/siret/prank/program/visualization/renderers/RendererTestFixtures.groovy diff --git a/README.md b/README.md index 90e4d88e..34d8540f 100644 --- a/README.md +++ b/README.md @@ -121,6 +121,10 @@ prank predict -c alphafold test.ds # use alphafold config and model (confi * **SAS points data**: coordinates and ligandability scores for solvent-accessible surface (SAS) points are saved in `visualizations/data/{protein_file}_points.pdb.gz`. Here: * Residue sequence number (position 23-26) represents the pocket rank (0 indicates no pocket). * B-factor column contains predicted ligandability score. + * **Opt-in tabular exports** (off by default): + * `-export_pocket_descriptors 1` → per-pocket geometric descriptors (volume, sphericity, radius of gyration, residue/atom counts, principal moments) in CSV/Arrow/Parquet. See [`documentation/export-pocket-descriptors.md`](documentation/export-pocket-descriptors.md). + * `-export_pocket_grid 1` → per-pocket 3D grid of points covering the empty space around predicted pockets, with optional per-grid-point descriptors (e.g. `volsite` pharmacophore indicators). See [`documentation/export-pocket-grid.md`](documentation/export-pocket-grid.md). + * `-vis_pocket_grid 1` → additional PyMOL/ChimeraX overlays of the grid for visualization. ### Configuration diff --git a/breaking-changes.md b/breaking-changes.md index 99e49134..9a808272 100644 --- a/breaking-changes.md +++ b/breaking-changes.md @@ -27,6 +27,18 @@ All changes of that type should be rare and should be all listed here. * For additional internal evaluation-criterion fixes during the 2.6 dev cycle see [`documentation/dev/evaluation-metric-fixes-2.6.md`](documentation/dev/evaluation-metric-fixes-2.6.md). +###### Pocket-descriptors export (opt-in feature) + +* Per-pocket descriptors `-export_pocket_descriptors` underwent a multi-column interface migration. The shipped default + list now contains **seven** descriptors (previously six), adds `principal_moments` (a 3-column descriptor emitting + `principal_moments.lambda1/lambda2/lambda3`), and reorders the existing six so `num_*` come first. + Scripts parsing the descriptors CSV/Arrow/Parquet output by **column name** are unaffected; + scripts parsing by **column index** need updating. See [`documentation/export-pocket-descriptors.md`](documentation/export-pocket-descriptors.md). +* New opt-in `-vis_pocket_grid` (renamed from `-export_pocket_grid_pml`) emits both PyMOL `.pml` and ChimeraX `.cxc` + overlay scripts. The two viz-tuning knobs were renamed for namespace consistency: + `pocket_grid_vis_volume_radius` → `vis_pocket_grid_volume_radius` and + `pocket_grid_vis_gaussian_iso` → `vis_pocket_grid_gaussian_iso`. Old names hard-fail at startup with no aliases. + ### 2.5.1 none diff --git a/documentation/export-pocket-descriptors.md b/documentation/export-pocket-descriptors.md index 76c4627c..a6495640 100644 --- a/documentation/export-pocket-descriptors.md +++ b/documentation/export-pocket-descriptors.md @@ -124,10 +124,15 @@ Implementations live under 4. **To include it in the default output**, also add the name to the `pocket_descriptors` default list in `Params.groovy`. The default is - declared explicitly rather than derived from `Registry.knownNames()` - so that adding a descriptor doesn't silently change every existing - user's output schema — that's intentional; skip step 4 if the new - descriptor is opt-in only. + declared explicitly (rather than derived from `Registry.knownNames()`) + so each addition to the default schema is a conscious choice — but + adding to the default IS a user-visible breaking change for anyone + parsing the output by column index. Two recommendations: + - Parse the descriptors file by column **name**, not by column index. + - When you add a descriptor to the default list, note it in + [`breaking-changes.md`](../breaking-changes.md). + + Skip step 4 if the new descriptor is opt-in only. INT columns return their value as a `double` that the writer downcasts at output time, matching the existing `TableData` convention. Implementations diff --git a/documentation/export-pocket-grid.md b/documentation/export-pocket-grid.md index 2c0c4c5d..b1cfb77a 100644 --- a/documentation/export-pocket-grid.md +++ b/documentation/export-pocket-grid.md @@ -96,6 +96,36 @@ Descriptor params: | `pocket_grid_volsite_radius` | `4.0` Å | Cutoff radius for the `volsite` indicator. Standard VolSite pharmacophore search distance. | | `pocket_grid_volsite_sigma` | `2.0` Å | Gaussian σ for `volsite_smooth`. Kernel truncated at `4σ`. | +### Adding a new per-grid-point descriptor + +Implementations live under +`src/main/groovy/cz/siret/prank/program/routines/predict/output/grid/descriptors/`. + +1. Implement the `PocketGridPointDescriptor` interface (`name`, `columnNames`, + `columnTypes`, `compute`). The shape mirrors the per-pocket + `PocketDescriptor` (see + [`export-pocket-descriptors.md`](export-pocket-descriptors.md#adding-a-new-descriptor) + for the full recipe), with two differences: + - **No `needsGrid()` method.** Every grid-point descriptor needs the grid + by definition — the grid is the substrate that defines what a grid point + is. The orchestrator always builds the grid when any grid-point + descriptor is selected. + - **No `AbstractScalarPocketDescriptor`-style adapter.** Both shipped + descriptors (`volsite`, `volsite_smooth`) are multi-column; if you add + the first scalar grid-point descriptor and it's the only one, implement + `columnNames()` as a single-element list and rely on the bare `name()` + output convention. If a second arrives, factor out an adapter then. + +2. Register in `PocketGridPointDescriptorRegistry`'s static initializer. The + registry rejects descriptors with duplicate `columnNames` at registration + time. + +3. Users opt in by name: `-pocket_grid_point_descriptors "volsite,my_new_descriptor"`. + +4. Default-empty is deliberate — adding a per-grid-point descriptor to the + default would expand the row count by columns × rows, which is materially + non-free (see cost rationale above). New descriptors should ship opt-in. + ## Parameters | Parameter | Default | Notes | @@ -151,7 +181,7 @@ The volume surface is rendered as a vdW-style surface (solvent probe = 0) of radius `vis_pocket_grid_volume_radius` (Å, auto-scaled to `0.85 × spacing` when the param is left at its `-1` sentinel; ≈ 1.02 Å at default spacing) around each grid point. The default sits just above -the 3D-diagonal merge threshold (`spacing × √3 / 2 ≈ 0.87 × spacing`), +the 3D-diagonal merge threshold (`spacing × √3 / 2 ≈ 0.866 × spacing`), so neighbors overlap in every direction and the surface reads as one clean continuous blob per pocket. Going much below `~spacing/2` leaves the spheres too disconnected for PyMOL's surface algorithm — most of diff --git a/misc/test-scripts/testsets.sh b/misc/test-scripts/testsets.sh index cbc5547f..527e910e 100755 --- a/misc/test-scripts/testsets.sh +++ b/misc/test-scripts/testsets.sh @@ -561,8 +561,8 @@ pocket_grid() { test ./prank.sh predict -f distro/test_data/1fbl.pdb -export_pocket_grid 1 -pocket_grid_format arrow.zst -out_subdir TEST/POCKET_GRID test ./prank.sh predict -f distro/test_data/1fbl.pdb -export_pocket_grid 1 -pocket_grid_format parquet -out_subdir TEST/POCKET_GRID - # Full descriptor menu - test ./prank.sh predict -f distro/test_data/1fbl.pdb -export_pocket_descriptors 1 -pocket_descriptors 'volume,sphericity,radius_of_gyration,num_residues,num_surface_atoms,num_grid_points' -out_subdir TEST/POCKET_GRID + # Full descriptor menu (all seven shipped, including principal_moments) + test ./prank.sh predict -f distro/test_data/1fbl.pdb -export_pocket_descriptors 1 -pocket_descriptors 'volume,sphericity,radius_of_gyration,num_residues,num_surface_atoms,num_grid_points,principal_moments' -out_subdir TEST/POCKET_GRID # Grid-free descriptors only — exercises the grid-build short-circuit (no # "PocketGrid built" log line should appear for these invocations). diff --git a/src/main/groovy/cz/siret/prank/program/routines/predict/output/PocketDescriptorsRows.groovy b/src/main/groovy/cz/siret/prank/program/routines/predict/output/PocketDescriptorsRows.groovy index ab3cc463..6d319071 100644 --- a/src/main/groovy/cz/siret/prank/program/routines/predict/output/PocketDescriptorsRows.groovy +++ b/src/main/groovy/cz/siret/prank/program/routines/predict/output/PocketDescriptorsRows.groovy @@ -78,7 +78,7 @@ final class PocketDescriptorsRows implements TableData { if (grid == null) { for (PocketDescriptor d : descriptors) { if (d.needsGrid()) { - throw new IllegalArgumentException( + throw new cz.siret.prank.program.PrankException( "Descriptor '${d.name()}' declares needsGrid()=true but a null " + "PocketGrid was passed to PocketDescriptorsRows. Either build the " + "grid upstream or drop this descriptor from -pocket_descriptors.") diff --git a/src/main/groovy/cz/siret/prank/program/routines/predict/output/PocketGridRows.groovy b/src/main/groovy/cz/siret/prank/program/routines/predict/output/PocketGridRows.groovy index 1411fba3..288c6d8c 100644 --- a/src/main/groovy/cz/siret/prank/program/routines/predict/output/PocketGridRows.groovy +++ b/src/main/groovy/cz/siret/prank/program/routines/predict/output/PocketGridRows.groovy @@ -40,10 +40,6 @@ final class PocketGridRows implements TableData { /** [rowIndex][descriptorColumn] — flat across all descriptors; null when no descriptors. */ private final double[][] descriptorValues - PocketGridRows(PocketGrid grid, boolean includeUnassigned) { - this(grid, includeUnassigned, null, null, Collections. emptyList()) - } - PocketGridRows(PocketGrid grid, boolean includeUnassigned, Protein protein, List pockets, List descriptorNames) { @@ -178,7 +174,7 @@ final class PocketGridRows implements TableData { double[] getColumn(int colIndex) { int n = rowPointIdx.length double[] out = new double[n] - if (colIndex == 3) { + if (colIndex == BASE_COLS - 1) { // pocket column — INT for (int i = 0; i < n; i++) out[i] = rowPocket[i] return out } diff --git a/src/main/groovy/cz/siret/prank/program/routines/predict/output/descriptors/PocketDescriptorRegistry.java b/src/main/groovy/cz/siret/prank/program/routines/predict/output/descriptors/PocketDescriptorRegistry.java index 3699bd7f..3a58fa77 100644 --- a/src/main/groovy/cz/siret/prank/program/routines/predict/output/descriptors/PocketDescriptorRegistry.java +++ b/src/main/groovy/cz/siret/prank/program/routines/predict/output/descriptors/PocketDescriptorRegistry.java @@ -45,6 +45,12 @@ public final class PocketDescriptorRegistry { */ public static void register(PocketDescriptor d) { List cols = d.columnNames(); + List types = d.columnTypes(); + if (cols.size() != types.size()) { + throw new IllegalStateException( + "Descriptor '" + d.name() + "' has columnNames.size()=" + cols.size() + + " but columnTypes.size()=" + types.size() + "; they must be parallel."); + } if (cols.size() > 1 && new HashSet<>(cols).size() != cols.size()) { throw new IllegalStateException( "Descriptor '" + d.name() + "' declares duplicate columnNames: " + cols); diff --git a/src/main/groovy/cz/siret/prank/program/routines/predict/output/descriptors/PrincipalMomentsDescriptor.java b/src/main/groovy/cz/siret/prank/program/routines/predict/output/descriptors/PrincipalMomentsDescriptor.java index ebbba072..a12b6c0b 100644 --- a/src/main/groovy/cz/siret/prank/program/routines/predict/output/descriptors/PrincipalMomentsDescriptor.java +++ b/src/main/groovy/cz/siret/prank/program/routines/predict/output/descriptors/PrincipalMomentsDescriptor.java @@ -107,9 +107,15 @@ public final class PrincipalMomentsDescriptor implements PocketDescriptor { return new double[] { l1, l2, l3 }; } - /** PSD eigenvalues should be ≥ 0; numerical noise can push them slightly negative. */ + /** + * PSD eigenvalues should be ≥ 0; numerical noise can push them slightly negative. + * NaN is also clamped to 0 as defense-in-depth: {@code NaN < 0} is false, so a NaN + * eigenvalue would otherwise slip through and propagate to the CSV. The upstream + * {@code GridGenerator.isFiniteBox} guard already rejects non-finite inputs, but + * a future code path could bypass it. + */ private static double clampNonNegative(double v) { - return v < 0d ? 0d : v; + return (v < 0d || !Double.isFinite(v)) ? 0d : v; } } diff --git a/src/main/groovy/cz/siret/prank/program/routines/predict/output/grid/PocketGridBuilder.java b/src/main/groovy/cz/siret/prank/program/routines/predict/output/grid/PocketGridBuilder.java index f0575c69..f2de97fe 100644 --- a/src/main/groovy/cz/siret/prank/program/routines/predict/output/grid/PocketGridBuilder.java +++ b/src/main/groovy/cz/siret/prank/program/routines/predict/output/grid/PocketGridBuilder.java @@ -120,7 +120,7 @@ public final class PocketGridBuilder { case "morph_closing": return new MorphologicalCloser(); case "none": return new NoOpFiller(); default: - throw new IllegalArgumentException( + throw new cz.siret.prank.program.PrankException( "Unknown pocket_grid_fill strategy: '" + strategy + "'. Expected one of: morph_closing, none."); } diff --git a/src/main/groovy/cz/siret/prank/program/routines/predict/output/grid/descriptors/PocketGridPointContext.java b/src/main/groovy/cz/siret/prank/program/routines/predict/output/grid/descriptors/PocketGridPointContext.java index 7226e5ea..d920d92a 100644 --- a/src/main/groovy/cz/siret/prank/program/routines/predict/output/grid/descriptors/PocketGridPointContext.java +++ b/src/main/groovy/cz/siret/prank/program/routines/predict/output/grid/descriptors/PocketGridPointContext.java @@ -23,4 +23,17 @@ public record PocketGridPointContext( @Nullable Pocket pocket, Protein protein, PocketGrid grid) { + + // Compact validator — limits the blast radius of an int-arg swap. Doesn't catch + // pointIndex ↔ pocketRank swapped when both happen to be non-negative, but does + // catch the common cases (negative index or rank from a misuse). + public PocketGridPointContext { + if (pointIndex < 0) { + throw new IllegalArgumentException("pointIndex must be >= 0 (got " + pointIndex + ")"); + } + if (pocketRank < 0) { + throw new IllegalArgumentException( + "pocketRank must be >= 0 (0 = unassigned; got " + pocketRank + ")"); + } + } } diff --git a/src/main/groovy/cz/siret/prank/program/routines/predict/output/grid/descriptors/PocketGridPointDescriptorRegistry.java b/src/main/groovy/cz/siret/prank/program/routines/predict/output/grid/descriptors/PocketGridPointDescriptorRegistry.java index b8951747..1a2f99cf 100644 --- a/src/main/groovy/cz/siret/prank/program/routines/predict/output/grid/descriptors/PocketGridPointDescriptorRegistry.java +++ b/src/main/groovy/cz/siret/prank/program/routines/predict/output/grid/descriptors/PocketGridPointDescriptorRegistry.java @@ -36,6 +36,12 @@ public final class PocketGridPointDescriptorRegistry { */ public static void register(PocketGridPointDescriptor d) { List cols = d.columnNames(); + List types = d.columnTypes(); + if (cols.size() != types.size()) { + throw new IllegalStateException( + "Descriptor '" + d.name() + "' has columnNames.size()=" + cols.size() + + " but columnTypes.size()=" + types.size() + "; they must be parallel."); + } if (cols.size() > 1 && new HashSet<>(cols).size() != cols.size()) { throw new IllegalStateException( "Descriptor '" + d.name() + "' declares duplicate columnNames: " + cols); diff --git a/src/test/groovy/cz/siret/prank/geom/samplers/GridGeneratorBetweenTest.groovy b/src/test/groovy/cz/siret/prank/geom/samplers/GridGeneratorBetweenTest.groovy index 1efdca20..4e6946ce 100644 --- a/src/test/groovy/cz/siret/prank/geom/samplers/GridGeneratorBetweenTest.groovy +++ b/src/test/groovy/cz/siret/prank/geom/samplers/GridGeneratorBetweenTest.groovy @@ -117,6 +117,24 @@ class GridGeneratorBetweenTest { } } + @Test + void nanCoordInSasPointsThrowsClearError() { + // GridGenerator's (Box, edge) ctor guards against NaN/Inf input — without it, + // IEEEremainder(NaN, edge) silently produces NaN origins and a NaN-everywhere + // lattice. This test pins the throw so a future refactor that drops the guard + // can't reintroduce silent NaN propagation. + Atoms atoms = new Atoms([carbonAt(0d, 0d, 0d)]) + Atoms sasWithNaN = new Atoms([ + new Point(0d, 0d, 0d) as Atom, + new Point(Double.NaN, 0d, 0d) as Atom + ]) + IllegalArgumentException e = assertThrows(IllegalArgumentException.class) { + GridGenerator.sampleGridPointsBetween(atoms, sasWithNaN, 1.0d, 3.0d, 0.5d) + } as IllegalArgumentException + assertTrue(e.message.toLowerCase().contains('non-finite'), + "expected non-finite-box error, got: ${e.message}") + } + @Test void returnedOriginMatchesGridShift() { // Sampler exposes the lattice origin it picked so downstream callers don't diff --git a/src/test/groovy/cz/siret/prank/program/routines/predict/output/PocketGridRowsTest.groovy b/src/test/groovy/cz/siret/prank/program/routines/predict/output/PocketGridRowsTest.groovy index 01c1ce90..74e9309a 100644 --- a/src/test/groovy/cz/siret/prank/program/routines/predict/output/PocketGridRowsTest.groovy +++ b/src/test/groovy/cz/siret/prank/program/routines/predict/output/PocketGridRowsTest.groovy @@ -45,7 +45,7 @@ class PocketGridRowsTest { @Test void multiPocketMembershipProducesMultipleRows() { // Point b is in both pockets → it appears twice (once per pocket). - PocketGridRows data = new PocketGridRows(buildTwoPocketGrid(), false) + PocketGridRows data = new PocketGridRows(buildTwoPocketGrid(), false, null, null, [] as List) assertEquals(4, data.rowCount) // 2 + 2 assignments assertEquals(['x', 'y', 'z', 'pocket'], data.header) } @@ -62,16 +62,16 @@ class PocketGridRowsTest { assigned.put(1, bits(0)) PocketGrid grid = new PocketGrid(new Atoms([a, unassigned]), 1.0d, 0d, 0d, 0d, idx, assigned) - PocketGridRows included = new PocketGridRows(grid, true) + PocketGridRows included = new PocketGridRows(grid, true, null, null, [] as List) assertEquals(2, included.rowCount) // 1 assigned + 1 unassigned - PocketGridRows omitted = new PocketGridRows(grid, false) + PocketGridRows omitted = new PocketGridRows(grid, false, null, null, [] as List) assertEquals(1, omitted.rowCount) } @Test void sortOrderIsPocketThenCoords() { - PocketGridRows data = new PocketGridRows(buildTwoPocketGrid(), false) + PocketGridRows data = new PocketGridRows(buildTwoPocketGrid(), false, null, null, [] as List) // Expected sort: (pocket=1, x=1,2), then (pocket=2, x=2,3). double[] r0 = data.getRow(0); assertEquals(1.0d, r0[0], 0.0d); assertEquals(1, (int) r0[3]) double[] r1 = data.getRow(1); assertEquals(2.0d, r1[0], 0.0d); assertEquals(1, (int) r1[3]) @@ -81,7 +81,7 @@ class PocketGridRowsTest { @Test void columnTypes() { - PocketGridRows data = new PocketGridRows(buildTwoPocketGrid(), false) + PocketGridRows data = new PocketGridRows(buildTwoPocketGrid(), false, null, null, [] as List) assertEquals(TableData.ColumnType.DOUBLE, data.getColumnType(0)) assertEquals(TableData.ColumnType.DOUBLE, data.getColumnType(1)) assertEquals(TableData.ColumnType.DOUBLE, data.getColumnType(2)) diff --git a/src/test/groovy/cz/siret/prank/program/routines/predict/output/descriptors/PocketDescriptorsTest.groovy b/src/test/groovy/cz/siret/prank/program/routines/predict/output/descriptors/PocketDescriptorsTest.groovy index 7d667f41..b476b548 100644 --- a/src/test/groovy/cz/siret/prank/program/routines/predict/output/descriptors/PocketDescriptorsTest.groovy +++ b/src/test/groovy/cz/siret/prank/program/routines/predict/output/descriptors/PocketDescriptorsTest.groovy @@ -270,6 +270,33 @@ class PocketDescriptorsTest { } } + /** + * Round-trip the register/unregister API: register a fixture, see it land in + * get/knownNames, unregister it, see it gone. Exercises the unregister code + * path that production callers never touch (it exists for tests + future + * descriptor plugins). + */ + @Test + void registryUnregisterRemovesAddedDescriptor() { + String fixtureName = "__test_unregister_fixture__" + PocketDescriptor fixture = new AbstractScalarPocketDescriptor() { + @Override String name() { fixtureName } + @Override protected ColumnType scalarType() { ColumnType.DOUBLE } + @Override protected double computeScalar(PocketGridContext ctx) { 0d } + } + PocketDescriptorRegistry.register(fixture) + try { + assertEquals(fixture, PocketDescriptorRegistry.get(fixtureName)) + assertTrue(PocketDescriptorRegistry.knownNames().contains(fixtureName)) + } finally { + PocketDescriptorRegistry.unregister(fixtureName) + } + assertFalse(PocketDescriptorRegistry.knownNames().contains(fixtureName)) + assertThrows(PrankException) { + PocketDescriptorRegistry.get(fixtureName) + } + } + @Test void registryListsKnownNames() { Set known = PocketDescriptorRegistry.knownNames() @@ -375,4 +402,46 @@ class PocketDescriptorsTest { assertEquals(rg * rg, sum, 1e-9d) } + /** + * Boundary between the short-circuit ({@code n < 2}) and the full path. + * Two distinct points span exactly one axis, so λ₁ > 0 and the other two + * eigenvalues are 0. Per-dim variance of {0, 2} = mean((-1)² + 1²) = 1. + */ + @Test + void principalMomentsAtExactlyTwoDistinctPointsHitsFullPath() { + List pts = [new Point(0d, 0d, 0d), new Point(2d, 0d, 0d)] + PocketGrid grid = gridOfPoints(pts) + TestPocket p = new TestPocket(); p.rank = 1 + double[] lambdas = new PrincipalMomentsDescriptor().compute(ctx(grid, p)) + assertEquals(1.0d, lambdas[0], 1e-9d) + assertEquals(0.0d, lambdas[1], 1e-9d) + assertEquals(0.0d, lambdas[2], 1e-9d) + } + + /** + * Two coincident points: cardinality=2 takes the full path, but every + * delta-from-centroid is 0, so the gyration tensor is zero and all + * eigenvalues come out zero. Verifies the full path handles the + * degenerate non-short-circuit case without producing NaN / negative + * eigenvalues from numerical noise. + */ + @Test + void principalMomentsOfTwoCoincidentPointsIsAllZeros() { + // LongIntHashMap can't hold two entries with the same packed key — for + // a true "two atoms at the same coord" fixture we need two distinct + // lattice slots whose Atom positions happen to coincide. Build by hand. + Atom a = new Point(0d, 0d, 0d) + Atom b = new Point(0d, 0d, 0d) + LongIntHashMap idx = new LongIntHashMap() + idx.put(PocketGrid.pack(0, 0, 0), 0) + idx.put(PocketGrid.pack(1, 0, 0), 1) // arbitrary distinct lattice slot + BitSet bs = new BitSet(); bs.set(0, 2) + Map assigned = [(1): bs] as LinkedHashMap + PocketGrid grid = new PocketGrid(new Atoms([a, b]), 1.0d, 0d, 0d, 0d, idx, assigned) + TestPocket p = new TestPocket(); p.rank = 1 + + double[] lambdas = new PrincipalMomentsDescriptor().compute(ctx(grid, p)) + assertArrayEquals([0.0d, 0.0d, 0.0d] as double[], lambdas, 1e-9d) + } + } diff --git a/src/test/groovy/cz/siret/prank/program/routines/predict/output/grid/MorphologicalCloserTest.groovy b/src/test/groovy/cz/siret/prank/program/routines/predict/output/grid/MorphologicalCloserTest.groovy index f8b6dfa7..812753b0 100644 --- a/src/test/groovy/cz/siret/prank/program/routines/predict/output/grid/MorphologicalCloserTest.groovy +++ b/src/test/groovy/cz/siret/prank/program/routines/predict/output/grid/MorphologicalCloserTest.groovy @@ -110,9 +110,11 @@ class MorphologicalCloserTest { } @Test - void respectsMaxIters() { + void maxItersZeroIsNoOp() { // With max_iters=0 the closer should return the raw shell unchanged - // (no iteration runs). + // (no iteration runs). Also pins the silent-non-convergence-warning fix + // in 0e044f6b — the warning must NOT fire when maxIters=0 (a valid + // "disable fill" configuration). PocketGrid grid = buildCubeGrid(2, 2, 2) int centerIdx = grid.latticeIndex.get(PocketGrid.pack(1, 1, 1)) BitSet raw = new BitSet() @@ -124,4 +126,20 @@ class MorphologicalCloserTest { assertFalse(result.get(centerIdx), "no fill when max_iters=0") } + @Test + void maxItersOneRunsExactlyOneIteration() { + // One iteration of fill should be enough to close a single-cell U concavity: + // the surrounded center cell has many filled neighbors and gets promoted on + // iter 0. Pins behavior between the no-op (0) and converged cases. + PocketGrid grid = buildCubeGrid(2, 2, 2) + int centerIdx = grid.latticeIndex.get(PocketGrid.pack(1, 1, 1)) + BitSet raw = new BitSet() + for (int i = 0; i < grid.pointCount; i++) { + if (i != centerIdx) raw.set(i) + } + + BitSet result = CLOSER.fill(raw, grid, 3, 1) + assertTrue(result.get(centerIdx), "the surrounded center should fill in one iter") + } + } diff --git a/src/test/groovy/cz/siret/prank/program/routines/predict/output/grid/PocketGridBuilderTest.groovy b/src/test/groovy/cz/siret/prank/program/routines/predict/output/grid/PocketGridBuilderTest.groovy index 5bd38ae7..bf0ef1ce 100644 --- a/src/test/groovy/cz/siret/prank/program/routines/predict/output/grid/PocketGridBuilderTest.groovy +++ b/src/test/groovy/cz/siret/prank/program/routines/predict/output/grid/PocketGridBuilderTest.groovy @@ -136,7 +136,7 @@ class PocketGridBuilderTest { TestPocket p = new TestPocket() p.rank = 1 p.sasPoints = sasAt(0d, 0d, 0d) - assertThrows(IllegalArgumentException) { + assertThrows(cz.siret.prank.program.PrankException) { PocketGridBuilder.build(protein, [p] as List, bad) } } diff --git a/src/test/groovy/cz/siret/prank/program/routines/predict/output/grid/descriptors/VolsiteGridPointDescriptorTest.groovy b/src/test/groovy/cz/siret/prank/program/routines/predict/output/grid/descriptors/VolsiteGridPointDescriptorTest.groovy index dbb69375..5a6288ea 100644 --- a/src/test/groovy/cz/siret/prank/program/routines/predict/output/grid/descriptors/VolsiteGridPointDescriptorTest.groovy +++ b/src/test/groovy/cz/siret/prank/program/routines/predict/output/grid/descriptors/VolsiteGridPointDescriptorTest.groovy @@ -3,12 +3,15 @@ package cz.siret.prank.program.routines.predict.output.grid.descriptors import cz.siret.prank.domain.Protein import cz.siret.prank.geom.Atoms import cz.siret.prank.geom.Point +import cz.siret.prank.program.params.Params import groovy.transform.CompileStatic import org.biojava.nbio.structure.Atom import org.biojava.nbio.structure.AtomImpl import org.biojava.nbio.structure.Element import org.biojava.nbio.structure.Group import org.biojava.nbio.structure.AminoAcidImpl +import org.junit.jupiter.api.AfterEach +import org.junit.jupiter.api.BeforeEach import org.junit.jupiter.api.Test import static org.junit.jupiter.api.Assertions.* @@ -28,6 +31,22 @@ class VolsiteGridPointDescriptorTest { private static final int AROMATIC = 0, CATION = 1, ANION = 2, HYDROPHOBIC = 3, ACCEPTOR = 4, DONOR = 5 + /** Radius the boundary tests build their atom distances around. */ + private static final double RADIUS = 4.0d + + private double savedRadius + + @BeforeEach + void setRadius() { + savedRadius = Params.inst.pocket_grid_volsite_radius + Params.inst.pocket_grid_volsite_radius = RADIUS + } + + @AfterEach + void restoreRadius() { + Params.inst.pocket_grid_volsite_radius = savedRadius + } + private static Atom atomAt(String element, String resName, String atomName, double x, double y, double z) { AtomImpl a = new AtomImpl() @@ -94,4 +113,23 @@ class VolsiteGridPointDescriptorTest { for (int i = 0; i < 6; i++) assertEquals(0d, out[i], 0d) } + @Test + void singleAtomWithTwoPharmacophoreFlagsLightsBothColumns() { + // ND1 in HIS sets BOTH donor and acceptor (VolSitePharmacophore.java). + // The 6-flag aggregator's "two flags from one atom" path is non-trivial; + // a regression that overwrites one with the other would slip through if + // every other test only exercises one-flag atoms. + Atom nd1His = atomAt("N", "HIS", "ND1", 1d, 0d, 0d) + double[] out = new VolsiteGridPointDescriptor().compute( + ctxAt(0d, 0d, 0d, new Atoms([nd1His]))) + + assertEquals(1d, out[DONOR]) + assertEquals(1d, out[ACCEPTOR]) + // The other four must remain off. + assertEquals(0d, out[AROMATIC]) + assertEquals(0d, out[CATION]) + assertEquals(0d, out[ANION]) + assertEquals(0d, out[HYDROPHOBIC]) + } + } diff --git a/src/test/groovy/cz/siret/prank/program/routines/predict/output/grid/descriptors/VolsiteSmoothGridPointDescriptorTest.groovy b/src/test/groovy/cz/siret/prank/program/routines/predict/output/grid/descriptors/VolsiteSmoothGridPointDescriptorTest.groovy index 02b90ff3..15caabaa 100644 --- a/src/test/groovy/cz/siret/prank/program/routines/predict/output/grid/descriptors/VolsiteSmoothGridPointDescriptorTest.groovy +++ b/src/test/groovy/cz/siret/prank/program/routines/predict/output/grid/descriptors/VolsiteSmoothGridPointDescriptorTest.groovy @@ -10,6 +10,7 @@ import org.biojava.nbio.structure.AtomImpl import org.biojava.nbio.structure.Element import org.biojava.nbio.structure.Group import org.biojava.nbio.structure.AminoAcidImpl +import org.junit.jupiter.api.AfterEach import org.junit.jupiter.api.BeforeEach import org.junit.jupiter.api.Test @@ -34,14 +35,28 @@ class VolsiteSmoothGridPointDescriptorTest { private static final int AROMATIC = 0, CATION = 1, ANION = 2, HYDROPHOBIC = 3, ACCEPTOR = 4, DONOR = 5 + private double savedSigma + @BeforeEach void setSigma() { + savedSigma = Params.inst.pocket_grid_volsite_sigma Params.inst.pocket_grid_volsite_sigma = SIGMA } - private static Atom atomAt(String atomName, String resName, double x, double y, double z) { + @AfterEach + void restoreSigma() { + // Restore the global Params singleton so test classes that run after this + // one (and that read pocket_grid_volsite_sigma without pinning it) aren't + // affected by our pin. + Params.inst.pocket_grid_volsite_sigma = savedSigma + } + + // Signature matches VolsiteGridPointDescriptorTest.atomAt (element, resName, atomName, x, y, z) + // — both tests build atoms the same way so calls don't get swapped between files. + private static Atom atomAt(String element, String resName, String atomName, + double x, double y, double z) { AtomImpl a = new AtomImpl() - a.element = Element.C + a.element = Element.valueOfIgnoreCase(element) a.name = atomName a.x = x; a.y = y; a.z = z Group g = new AminoAcidImpl() @@ -59,7 +74,7 @@ class VolsiteSmoothGridPointDescriptorTest { @Test void weightAtZeroDistanceIsOne() { // exp(0) = 1.0 exactly. Atom name "C" is hydrophobic. - Atom c = atomAt("C", "ALA", 0d, 0d, 0d) + Atom c = atomAt("C", "ALA", "C", 0d, 0d, 0d) double[] out = new VolsiteSmoothGridPointDescriptor().compute( ctxAt(0d, 0d, 0d, new Atoms([c]))) assertEquals(1.0d, out[HYDROPHOBIC], DELTA) @@ -69,7 +84,7 @@ class VolsiteSmoothGridPointDescriptorTest { void weightAtSigmaMatchesGaussianFormula() { // At distance r = σ, weight = exp(-r²/(2σ²)) = exp(-1/2) ≈ 0.6065. double sigma = SIGMA - Atom c = atomAt("C", "ALA", sigma, 0d, 0d) + Atom c = atomAt("C", "ALA", "C", sigma, 0d, 0d) double[] out = new VolsiteSmoothGridPointDescriptor().compute( ctxAt(0d, 0d, 0d, new Atoms([c]))) assertEquals(Math.exp(-0.5d), out[HYDROPHOBIC], DELTA) @@ -80,8 +95,8 @@ class VolsiteSmoothGridPointDescriptorTest { // Two hydrophobic atoms at distance σ each → sum = 2 × exp(-0.5). double sigma = SIGMA Atoms protein = new Atoms([ - atomAt("C", "ALA", sigma, 0d, 0d), - atomAt("C", "ALA", 0d, sigma, 0d), + atomAt("C", "ALA", "C", sigma, 0d, 0d), + atomAt("C", "ALA", "C", 0d, sigma, 0d), ]) double[] out = new VolsiteSmoothGridPointDescriptor().compute( ctxAt(0d, 0d, 0d, protein)) @@ -94,7 +109,7 @@ class VolsiteSmoothGridPointDescriptorTest { // exactly 4σ IS included and contributes exp(-(4σ)²/(2σ²)) = exp(-8) ≈ 3.354e-4. // Pins the boundary semantic (cutoff is inclusive, not strict). double sigma = SIGMA - Atom c = atomAt("C", "ALA", 4d * sigma, 0d, 0d) + Atom c = atomAt("C", "ALA", "C", 4d * sigma, 0d, 0d) double[] out = new VolsiteSmoothGridPointDescriptor().compute( ctxAt(0d, 0d, 0d, new Atoms([c]))) assertEquals(Math.exp(-8d), out[HYDROPHOBIC], DELTA) @@ -105,7 +120,7 @@ class VolsiteSmoothGridPointDescriptorTest { // 4σ is the hard cutoff (cutoutSphere is the gate). At 5σ the atom isn't even // in the kdtree result. Zero contribution. double sigma = SIGMA - Atom c = atomAt("C", "ALA", 5d * sigma, 0d, 0d) + Atom c = atomAt("C", "ALA", "C", 5d * sigma, 0d, 0d) double[] out = new VolsiteSmoothGridPointDescriptor().compute( ctxAt(0d, 0d, 0d, new Atoms([c]))) assertEquals(0d, out[HYDROPHOBIC], DELTA) diff --git a/src/test/groovy/cz/siret/prank/program/visualization/renderers/PocketGridChimeraXRendererTest.groovy b/src/test/groovy/cz/siret/prank/program/visualization/renderers/PocketGridChimeraXRendererTest.groovy index 2a9429c0..378e5190 100644 --- a/src/test/groovy/cz/siret/prank/program/visualization/renderers/PocketGridChimeraXRendererTest.groovy +++ b/src/test/groovy/cz/siret/prank/program/visualization/renderers/PocketGridChimeraXRendererTest.groovy @@ -21,24 +21,8 @@ class PocketGridChimeraXRendererTest { @TempDir Path tempDir - private static BitSet bits(int... values) { - BitSet b = new BitSet() - for (int v : values) b.set(v) - return b - } - private static PocketGrid tinyGrid() { - Atom a = new Point(0d, 0d, 0d) - Atom b = new Point(1d, 0d, 0d) - Atom c = new Point(2d, 0d, 0d) - LongIntHashMap idx = new LongIntHashMap() - idx.put(PocketGrid.pack(0, 0, 0), 0) - idx.put(PocketGrid.pack(1, 0, 0), 1) - idx.put(PocketGrid.pack(2, 0, 0), 2) - Map assigned = new LinkedHashMap<>() - assigned.put(1, bits(0, 1)) - assigned.put(2, bits(1, 2)) - return new PocketGrid(new Atoms([a, b, c]), 1.0d, 0d, 0d, 0d, idx, assigned) + return RendererTestFixtures.tinyGrid() } @Test diff --git a/src/test/groovy/cz/siret/prank/program/visualization/renderers/PocketGridPymolRendererTest.groovy b/src/test/groovy/cz/siret/prank/program/visualization/renderers/PocketGridPymolRendererTest.groovy index 302bf4d9..de45edaa 100644 --- a/src/test/groovy/cz/siret/prank/program/visualization/renderers/PocketGridPymolRendererTest.groovy +++ b/src/test/groovy/cz/siret/prank/program/visualization/renderers/PocketGridPymolRendererTest.groovy @@ -26,24 +26,8 @@ class PocketGridPymolRendererTest { @TempDir Path tempDir - private static BitSet bits(int... values) { - BitSet b = new BitSet() - for (int v : values) b.set(v) - return b - } - private static PocketGrid tinyGrid() { - Atom a = new Point(0d, 0d, 0d) - Atom b = new Point(1d, 0d, 0d) - Atom c = new Point(2d, 0d, 0d) - LongIntHashMap idx = new LongIntHashMap() - idx.put(PocketGrid.pack(0, 0, 0), 0) - idx.put(PocketGrid.pack(1, 0, 0), 1) - idx.put(PocketGrid.pack(2, 0, 0), 2) - Map assigned = new LinkedHashMap<>() - assigned.put(1, bits(0, 1)) - assigned.put(2, bits(1, 2)) - return new PocketGrid(new Atoms([a, b, c]), 1.0d, 0d, 0d, 0d, idx, assigned) + return RendererTestFixtures.tinyGrid() } @Test @@ -162,7 +146,7 @@ class PocketGridPymolRendererTest { new Atoms([new Point(0d, 0d, 0d) as Atom]), 3.0d, 0d, 0d, 0d, makeIdx(0), - [(1 as Integer): bits(0)] as LinkedHashMap) + [(1 as Integer): RendererTestFixtures.bits(0)] as LinkedHashMap) PocketGridPymolRenderer.render(coarse, 2.5d, GAUSSIAN_ISO, tempDir.toString(), "coarse") String pml = new File("${tempDir}/visualizations/coarse_pocket_grid.pml").text assertTrue(pml.contains("alter pocket_vol_*, vdw=2.500"), diff --git a/src/test/groovy/cz/siret/prank/program/visualization/renderers/RendererTestFixtures.groovy b/src/test/groovy/cz/siret/prank/program/visualization/renderers/RendererTestFixtures.groovy new file mode 100644 index 00000000..5375a3ad --- /dev/null +++ b/src/test/groovy/cz/siret/prank/program/visualization/renderers/RendererTestFixtures.groovy @@ -0,0 +1,46 @@ +package cz.siret.prank.program.visualization.renderers + +import com.carrotsearch.hppc.LongIntHashMap +import cz.siret.prank.geom.Atoms +import cz.siret.prank.geom.Point +import cz.siret.prank.program.routines.predict.output.grid.PocketGrid +import groovy.transform.CompileStatic +import org.biojava.nbio.structure.Atom + +/** + * Shared test fixtures for the pocket-grid renderer tests. Both + * {@code PocketGridChimeraXRendererTest} and {@code PocketGridPymolRendererTest} + * exercised an identical 3-atom / 2-pocket-overlap grid; extracted here so + * a fixture change touches one site. + */ +@CompileStatic +final class RendererTestFixtures { + + private RendererTestFixtures() {} + + static BitSet bits(int... values) { + BitSet b = new BitSet() + for (int v : values) b.set(v) + return b + } + + /** + * 3 grid points (a=0,0,0 — b=1,0,0 — c=2,0,0), spacing 1.0, + * pocket 1 = {a, b}, pocket 2 = {b, c}. Used by the renderer tests to + * verify per-pocket coloring, multi-membership behavior, etc. + */ + static PocketGrid tinyGrid() { + Atom a = new Point(0d, 0d, 0d) + Atom b = new Point(1d, 0d, 0d) + Atom c = new Point(2d, 0d, 0d) + LongIntHashMap idx = new LongIntHashMap() + idx.put(PocketGrid.pack(0, 0, 0), 0) + idx.put(PocketGrid.pack(1, 0, 0), 1) + idx.put(PocketGrid.pack(2, 0, 0), 2) + Map assigned = new LinkedHashMap<>() + assigned.put(1, bits(0, 1)) + assigned.put(2, bits(1, 2)) + return new PocketGrid(new Atoms([a, b, c]), 1.0d, 0d, 0d, 0d, idx, assigned) + } + +}