p2rank/misc/dev/audit-post-2.5.1.md

# Post-2.5.1 Audit Findings

Punch-list captured from a 10-agent audit of all changes between tag `2.5.1`
and `develop` (branch `develop`, ~218 commits, ~523 files). Each item is
grouped by severity. File paths are repo-relative; line numbers reflect
state at audit time and may drift.

This is a tech-debt / triage backlog — items here have not been fixed.
Resolve items individually as the surrounding code is touched, or schedule
focused cleanups.

---

## Highest-priority bugs (real or near-real)

- **`VoxelHashAssigner` cell-prune lower bound is too aggressive.**
  `src/main/groovy/cz/siret/prank/program/routines/predict/output/grid/assign/VoxelHashAssigner.java:58`
  uses `(di²+dj²+dk²)·spacing²` as a lower bound that ignores the sub-cell offset
  of `q`. Concrete miss with shipping defaults: `q=(0.59,0,0)`, `spacing=1.2`,
  `cutoff=2.5` skips a cell whose true distance is ~2.17 Å. Silently breaks the
  "both assigners agree" invariant documented on `PocketAssigner.java:23-26`.
  Existing test (`PocketGridBuilderTest.groovy:158`) misses it because its
  single SAS point lands exactly on a lattice center. Fix: correct the lower
  bound (`max(0, |di|*spacing − spacing/2)` per axis), or drop the prune.

- **Lazy-init race on singleton energy features.**
  `MethylEnergyFeature.groovy:28-40`, `MethylEnergyCloud{,X,XFull,X2,X2Full}SF.groovy:67-80`,
  `AbstractProbeEnergyFeature.groovy:47-59,66`.
  `calculator`/`config` are non-volatile fields on registry singletons; multiple
  worker threads can construct (and observe partially constructed) calculators.
  `ConcurrencyTest.groovy:27-28` constructs the calculator on the main thread
  before spawning threads, so the test does NOT cover the race it was meant to
  validate. Fix: `volatile` + DCL, or do one-shot init under a synchronized guard
  in `preProcessProtein`; update the concurrency test to construct under contention.

- **Coulomb plumbing is dead code.**
  `EnergyCalculator.getAtomCharge` always returns 0
  (`src/main/groovy/cz/siret/prank/features/implementation/energy2/calc/EnergyCalculator.groovy:351-357`).
  `enableCoulomb`, `dielectricConstant`, `coulombConstant` are structurally
  inert. `testCationProbeIncludesBothLJAndCoulombTerms`
  (`EnergyCalculatorTest.groovy:194-209`) is a false positive against its own
  name. Either wire charges, or rip the Coulomb path out.

- **PUResNet `surfaceAtoms` come from a separate `Structure`.**
  `src/main/groovy/cz/siret/prank/domain/loaders/pockets/PUResNetLoader.groovy:50,55`.
  The Concavity fix (`c143e0fa`) only re-routed the `Prediction` binding;
  PUResNet has the same identity-mismatch class still uncaught (atoms are from
  a sub-PDB, not from `queryProtein`). ConcavityLoader's own `cutoutShell` still
  runs against the residue subset rather than `queryProtein.exposedAtoms`. Fix:
  re-link atoms by PDB serial against `queryProtein.allAtoms.getByID`, mirroring
  `FPocketLoader.groovy:137`.

- **`AhojUbsSiteParser` will crash on "older full" CSVs.**
  `src/main/groovy/cz/siret/prank/domain/loaders/AhojUbsSiteParser.groovy:46`
  uses `pocket_class` as the sole marker but then unconditionally reads
  `rg`/`n_unp_pockets`/`n_unp_pockets_multichain` in `AhojSiteInfo.fromCsvRecord`.
  Commons CSV throws `IllegalArgumentException` for unmapped header names.
  Fix: add `record.isMapped(name)` guards, or pick a marker column guaranteed
  present in every variant.

- **`Ligands.loadLigandsFromSeparateFiles` doesn't filter cofactors.**
  `src/main/groovy/cz/siret/prank/domain/Ligands.groovy:140-198`.
  When `load_ligands_from_separate_files=true`, the cofactor branch in
  `Protein.loadStructure` (`Protein.groovy:574-592`) adds cofactor atoms to
  `proteinAtoms`, but the loader has no `.findAll { !loaderParams.isCofactor(it) }`
  guard. The cofactor becomes an *ignored Ligand* with contact distance ≈ 0.
  Self-acknowledged as cosmetic in `documentation/cofactors.md:421-429` and
  dev doc Known Limitation #1, but no test exercises it.

- **Aromatic-probe energy cap is applied before the cosine switch.**
  `EnergyCalculator.groovy:248-250` clips pre-switch, then multiplies by
  `neighbor.switchValue` at line 291. May be intended, but contradicts inline doc
  and `testAromaticRingEnergyCap` (`EnergyCalculatorTest.groovy:132-146`) doesn't
  exercise the divergence (single atom inside RON window). Decide per-atom vs
  per-point capping and align doc + test.

- **Centroid-source divergence across pocket loaders.**
  Pocketeer uses the server-supplied JSON `centroid`; FPocket uses voronoi
  centerOfMass; Concavity/PUResNet use geometric centroid post-fix;
  Seq2Pocket/SwinSite use `gridPoints.centroid`/`surfaceAtoms.centroid`. No
  documented contract for what "centroid" means across loaders. Either document
  the policy or normalize to geometric.

- **`RescorePocketsRoutine.checkDataset` has an empty truthy branch.**
  `src/main/groovy/cz/siret/prank/program/routines/predict/RescorePocketsRoutine.groovy:48-56`
  has `if (runFpocketAdHoc) { /* comment */ }` — the check appears inverted.
  Re-verify intended semantics.

---

## Inconsistencies (renames / strategy lookups / cross-format / code vs docs)

- **`NewPymolRenderer` class name is stale** — `NewPymolRenderer.groovy:28`.
  Two distinct active classes (`NewPymolRenderer` vs `PymolRenderer`). The
  "New" prefix predates a refactor. The cofactor block depends on a static
  method on the misnamed class (`PymolRenderer.groovy:147`). Rename.

- **Assigner vs filler lookup styles disagree.** Assigners go through
  `PocketAssignerRegistry`; fillers use a hand-written switch
  (`PocketGridBuilder.java:118-127`) plus a hard-coded `['morph_closing','none']`
  list (`Main.groovy:223`). Two places to keep in sync per new filler.

- **`NoOpFiller` returns input BitSet without cloning, `MorphologicalCloser`
  clones.** Mixed ownership semantics; `PocketShapeFiller.java:30` contract is
  silent. Risk of silent corruption if a future caller mutates the per-pocket
  BitSet under `fill=none`.

- **`SphericityDescriptor` degenerate convention diverges** —
  `SphericityDescriptor.java:32,48`. Empty pocket → 0.0, single-point pocket → 1.0.
  Volume / RadiusOfGyration / PrincipalMoments / NumGridPoints return 0 for both.
  Joining columns: one says "perfectly spherical", others say "empty".

- **`PocketGridPdbSidecar.write` called by both renderers** —
  `PocketGridPymolRenderer.groovy:121` + `PocketGridChimeraXRenderer.groovy:157`.
  When both are enabled, same sidecar PDB is written twice (identical content).

- **CSV `NaN`/`INF` handling is unspecified.** `Formatter` emits `NaN`/`∞`;
  INT columns silently coerce NaN to 0 (`TableExporter.groovy:142`). Document
  in `documentation/export-pocket-descriptors.md` and `export-points.md`, or
  pick a canonical sentinel.

- **Sort-direction comment in `PrincipalMomentsDescriptor.java:96-101`** says
  "Sort descending." while `Arrays.sort` is ascending. Downstream indexing
  compensates; comment is misleading.

- **Cofactor case-sensitivity mismatch.** `CofactorHandler.parseOne`
  (`CofactorHandler.groovy:325-335`) uppercases; `Dataset.LigandDefinition.parse`
  (`Dataset.groovy:761-805`) does not. `Params.groovy:536-537` doc claims
  case-sensitive while cofactors are normalized.

- **`distro/prank.bat` misses JVM flags.** Lines 11-13 set only the three
  `--add-opens`. Missing `--enable-native-access=ALL-UNNAMED` and the
  Java-23+ `--sun-misc-unsafe-memory-access=allow` block that `distro/prank`
  and `prank.sh` apply. zstd-jni native warnings + future-Java compatibility
  gap on Windows.

- **`atomRoleCache`/`atomChargeCache` keyed by BioJava `Atom` identity.**
  `EnergyCalculator.groovy:131-132`. Atoms across proteins are distinct
  identities; cache never hits across proteins and grows monotonically — slow
  leak for long-running calculators.

- **`computeEnergyForPoint` returns `List<Double>` (boxed).**
  `EnergyCalculator.groovy:146-179`. Boxing per neighbor per probe in the hot
  loop. Legacy `LJEnergyCalculator.computeEnergyForPoint` returns `double`.

- **`Evaluation.closestPocket()` ignores `site_eval_sas_pts_as_atoms`.**
  `Evaluation.groovy:81` doc says "considering DCA measure" but uses
  `site.atoms.dist(p.centroid)` unconditionally. Diverges from DCA semantics
  when the param is enabled.

- **`Evaluation.getStats()` returns `LinkedHashMap`** (comment claims "keep
  insertion order"), but the immediate caller `EvalResults.getStats()` puts
  everything into a `TreeMap` (`EvalResults.groovy:189`) — insertion order is
  lost. Either drop the misleading comment or use `LinkedHashMap` downstream.

- **PyMOL pocket-grid renderer iterates `1..maxRank`; ChimeraX iterates
  `perPocketBasenames.keySet()`.**
  `PocketGridPymolRenderer.groovy:167,201,242`.
  Cosmetic-only: P2Rank ranks pockets contiguously (every `predict`-path and
  in-tree loader except `SiteHoundLoader` assign `i++`/`rank++`), and the
  sidecar PDB strips ranks whose `filled` BitSet is empty. PyMOL therefore
  emits empty `pocket_grid_N`/`pocket_vol_N`/`pocket_gauss_N`/`pocket_hull_N`
  objects when the assigner produced no points for a small pocket — they
  render as invisible but clutter the Models panel. Mirror the ChimeraX
  iteration pattern (`a3efd084`) for parity; not a correctness fix.

- **PyMOL grid `solvent_radius=0` vs ChimeraX non-zero probe.**
  `PocketGridPymolRenderer.groovy:189-190` vs `PocketGridChimeraXRenderer.groovy:264`.
  `vis_pocket_grid_volume_radius` means different things to the two renderers.
  Documented in code; not in the param help.

- **`pocket=0` (unassigned) points never reach the PDB sidecar.**
  `PocketGridPdbSidecar.java:56-60` iterates `grid.getPocketToPointIndices()`
  only. When `-pocket_grid_include_unassigned 1`, CSV/Parquet has the rows
  but visualization silently drops them. Document or extend.

---

## Doc / config drift

- **README badge stuck at 2.5.1.** `README.md:11` vs `build.gradle:25`
  (`2.6.0-dev.9`). Typo `./make-disro.sh` at `README.md:225`.

- **Removed flags/commands still referenced in docs:**
  - `documentation/random-examples.md:7,12,17,22` — singular `-conservation_dir`
    (removed in 2.2, renamed to `-conservation_dirs`).
  - `documentation/hidden-commands.md:65` — `fasta-mask` (actual: `fasta-masked`).
  - `documentation/hidden-commands.md:83` — `analyze reduce-to-chains`
    (actual: `transform reduce-to-chains`).
  - `documentation/training-score-transformers.md:4` — `*_pockets.csv` (actual:
    `*_predictions.csv`).

- **`src/main/resources/help.txt` is severely outdated.** Lists only 5 commands;
  `Main.groovy:621-645` registers 12+. Typos `dafault`, `comamnd`. Doesn't
  mention any 2.6 loader rescorers despite `documentation/rescoring.md`
  describing 10.

- **`distro/config/default.groovy:128-130`** comments `ignore_het_groups` as
  "considered cofactor". After `-cofactors` shipped in 2.6, this is actively
  misleading. Same comment in `distro/config/default_rescore.groovy:120-122`.

- **`distro/config/default.groovy` is missing new params:**
  `cofactors`, `cofactor_max_protein_dist`, `aa_mapping`, `vis_highlight_cofactors`.

- **`documentation/cofactors.md:190,389`** say "logs an INFO warning"; code
  logs `WARN` (`CofactorHandler.groovy:222,261`).
  `documentation/dev/cofactors.md:36,39` also describe R11/R14 as "INFO"
  while line 69 of the same file documents the promotion to WARN.

- **`Params.groovy:536-537`** doc on cofactor matching is stale (claims
  case-sensitive — actually normalized).

- **`feature-setup.md:8`** still says "P2Rank version: 2.4"; collapsibles
  hardcode `P2Rank 2.3-dev.1`. Line 120 says "providing custom tables is not
  implemented yet (as for P2Rank 2.4.2)" while the very next bullet describes
  the `csv` feature that does this.

- **`feature-setup.md:125`** typo `{peorein_file_name}`.

- **`documentation/aa-mapping.md:21`** typo `ommited`, extra space before comma.

- **`documentation/readme.md`** index misses `cofactors.md`, `conservation.md`,
  `export-pocket-grid.md`, `export-pocket-descriptors.md`.

- **CI matrix is `17,21,25,26` only** (`.github/workflows/develop.yml:23`).
  README claims "Java 17 or later (tested up to Java 25)"; 18–20/22/23/24 not
  exercised; "tested up to Java 25" lags the now-present 26.

- **Distribution switched temurin → oracle** (`develop.yml:31`, commit
  `1997ab94`). No doc note explains the choice.

- **`PocketDescriptor.java:29-31` "fits in i32" contract** is unenforced;
  a descriptor producing `1e20` for an INT column silently emits
  `Integer.MAX_VALUE` or wraps. Add `Math.toIntExact` at the writer.

- **`PointExportData.create()` doc says "(for predict mode)"**
  (`PointExportData.groovy:141`) but it's also used by `rescore`
  (`ModelBasedRescorer.groovy:97,168`).

---

## Stale comments / dead code

- **"Immutable calculator instance" comments in 6 energy feature files** after
  the lazy-init refactor: `MethylEnergyFeature.groovy:22`,
  `MethylEnergyCloud{,X,XFull,X2,X2Full}SF.groovy:27/30`.

- **`EnergyCalculatorConfig.roleRulesCSV` + `role-rules.csv` resource are
  wired but read nowhere.** `EnergyCalculatorConfig.groovy:28,40,143,162-164`.
  `AtomRole.classify` is hardcoded.

- **`PocketShapeFiller.java:24` and `Params.groovy:896-897`** use the term
  "surface atom"; actual inputs are SAS points (pre-SAS-revision wording).

- **`LoaderParams.groovy:60-66`** commented-out copy constructor;
  `:20-22` stale `TODO get rid of this global variable` on `ignoreLigandsSwitch`.

- **References to gitignored `local/PLAN_COFACTORS*.md`** in committed code:
  `CofactorPipelineTest.groovy:38`, `CofactorAnalyzeTest.groovy:17`,
  `documentation/dev/cofactors.md:6`.

- **`FPocketLoader.groovy:149`**: `pocket.centroid = pocket.voronoiCenters.centerOfMass`
  is dead because the `getCentroid` override at line 42 always recomputes.

- **`FPocketLoader.groovy:155`** `// probably not needed` (years old);
  `:142` fpocket3 TODO.

- **`ConcavityLoader.groovy:74`** `// TODO XXX` is meaningless;
  `:91` `///X correctsorting…` typo log marker; the class is not `@CompileStatic`.

- **`PUResNetLoader.groovy:55`** truncated comment mid-sentence (`// not all
  of them are necessarily on the surface but it s`).
  `:35` parameter named `liganatedProtein` but expects queryProtein.
  `:23-28` javadoc is a stub.

- **`PocketeerLoaderTest`** missing `predictionIsBoundToQueryProtein` and
  empty-input tests (every other new loader test has both).

- **`PocketGridBuilder.java:97-99`** "monomorphic dispatch, JIT-friendly"
  comment is wrong — bimorphic with two impls registered.

- **`PredictionPair.groovy:61,78,107,109`** still uses `criterium` after
  class rename (commit `2de315e9`).

- **`Evaluation.groovy:484`** rank-sentinel comment says 0; actual sentinel
  is `-1` (`PredictionPair.rankOfIdentifiedPocket`).

- **`Evaluation.groovy:132`** comment "Clear SAS points cache" — actually
  clears per-site lazy lookup, not the EvalContext cache.

- **`MethylEnergyFeature.groovy:67-70`** dangling try/catch skeleton;
  `:55` commented alternative path.

- **`prank.sh:13-15`** commented `-XX:+UseConcMarkSweepGC` (removed in JDK 14).

- **`misc/development-notes.md`** is down to a single 6-line note —
  merge into `misc/dev/technical-debt.md` or delete.

- **`distro/config/default_rescore.groovy:46`** `//== FAETURES` typo.

- **`distro/prank.bat:14`** last `set "JAVA_OPTS=%JAVA_OPTS%"` is a no-op.

- **`PocketGridChimeraXRenderer.groovy:81`** says PyMOL transparency is 0.7;
  actual `PROTEIN_TRANSPARENCY` in `PocketGridPymolRenderer.groovy:76` is 0.5.

- **`PocketGridBuilder.java:54-56`** meta-commentary about
  `Atoms.union`-vs-`Atoms.join` choice — reads as a leftover review note.

- **`VolsiteGridPointDescriptorTest.groovy:81`** comment claims testing the
  default `pocket_grid_volsite_radius=4.0`, but the test pins `RADIUS=4.0`
  via `@BeforeEach`.

- **`PocketDescriptorsTest.groovy:110-112`** arithmetic in the comment
  (`≈ sqrt(2)*5 ≈ 7.07`) doesn't match the geometry (centroid at (4.5,4.5,0),
  max dist ≈ 6.36). Asserted ratio still passes.

- **`AbstractScalarPocketDescriptor.java:21-23`** comment says "both shipped
  descriptors are multi-column" — accurate today, will silently lie when a
  scalar grid-point descriptor is added.

---

## Test-isolation gaps

- **`CofactorIntegrationTest.groovy:40-43`** mutates
  `LoaderParams.ignoreLigandsSwitch` in `@BeforeAll` without
  `@Isolated`/`@ResourceLock("Params")` and without saving/restoring. Compare
  `CofactorPipelineTest.groovy:55-65` and `CofactorAnalyzeTest.groovy:32-44`
  which do both correctly.

- **`LigandMatchingTest`** snapshots `Params.INSTANCE` but is not
  `@Isolated`/`@ResourceLock`-annotated.

- **`VolsiteGridPointDescriptorTest` / `VolsiteSmoothGridPointDescriptorTest`**
  mutate `Params.INSTANCE` in `@BeforeEach/@AfterEach` without the project's
  standard `@Isolated`/`@ResourceLock("Params")` annotations.

- **`PocketDescriptorRegistry` / `PocketGridPointDescriptorRegistry`** —
  `NamedRegistryHelper`-backed `LinkedHashMap` is not synchronized. The
  public `register/unregister` is exposed for tests but lacks a
  thread-safety note. If a future `register` happens during a feature
  extractor read, behavior is undefined.

---

## Performance nits

- **`computeEnergyForPoint` boxes doubles** (above).

- **`EnergyCalculator.groovy:170`** `config.probeParams[probe]` `EnumMap.get`
  per neighbor per probe; hoist to a pre-sized `ProbeParams[]` indexed by
  `probeIdx`.

- **`PocketGridBuilder.java:83`** `Map<Integer, BitSet> pocketToPointIndices`
  uses boxed `Integer` keys while the rest of the file goes to lengths to
  avoid boxing (`LongIntHashMap`, primitive `int[]`). Use `IntObjectHashMap`.

- **`MorphologicalCloser.fill` clones empty rawShell**
  (`MorphologicalCloser.java:30`); unnecessary, inconsistent with `NoOpFiller`.

- **`KdTreeAssigner.computeRawShell` dedup branch is dead**
  (`KdTreeAssigner.java:41`): every atom returned by the KD tree is in
  `latticeIndex` by construction.

---

## Top-5 if you only fix five things

1. **Fix `VoxelHashAssigner` cell-prune lower bound** (or drop it and rely on
   the post-fetch distance check). Restores the assigner-strategy equivalence
   the docs promise.
2. **Make energy-feature lazy-init actually thread-safe**
   (`MethylEnergyFeature`, `AbstractProbeEnergyFeature`); fix `ConcurrencyTest`
   to construct calculators under contention.
3. **Guard `AhojSiteInfo.fromCsvRecord` with `record.isMapped(...)`** for the
   new `rg`/`n_unp_pockets[_multichain]` columns, so the parser doesn't crash
   on older "full" CSVs.
4. **Re-link `PUResNetLoader.surfaceAtoms` to `queryProtein`** by PDB serial
   (mirror `FPocketLoader.groovy:137`); same identity-mismatch class as the
   Concavity fix.
5. **README/help.txt/`distro/prank.bat` trio**: bump the version badge, fix
   the `./make-disro.sh` typo, regenerate `help.txt` to list current commands,
   and bring Windows launcher JVM flags up to parity with the Bash launchers.