Add a PyRosetta unit test for BindCraft (#753)
This doesn't test all of BindCraft, it just attempts to test the
PyRosetta portions of it, to make sure we don't have future regressions
on the use. (Like what was seen with PR #749)
One limitation is the lack of dalphaball. BindCraft uses it, but the I
do not think it's easy to use on the Rosetta test server. It's turned
off for this test, and hopefully not exercising the dalphaball-related
code doesn't cause too many issues.
Fix Segfault with backrub. (#758)
While BackrubProtocol clears the BackrubMover's segments between
proteins, the bond_angle_map_ data set by add_segment() is not.
This means that when you apply backrub to a large protein and then to a
small protein, the residue numbers from the large protein cause a
segmentation fault in optimize_branch_angles() when the residual data in
bond_angle_map_ is applied to the small protein.
Clearing bond_angle_map_ when we clear the segments addresses the issue.
Increase timeout for `T021_Pose_Methods` sub-test (#748)
The `T021_Pose_Methods.py` PyRosetta sub-test sometimes fails on the
hikaru-2 daemon on the Benchmark server seemingly due to reaching the 60
second timeout, likely due to a network filesystem (NFS) share or slow
loading. This PR adjusts the timeout to 5 minutes.
Caching environment manager version in PyRosettaCluster full simulation records (#747)
This quick PR adds a caching mechanism for the configured environment
manager's version string in the *PyRosettaCluster* full simulation
record. The captured version string facilitates environment
reproducibility when using Pixi or uv, as described in the accompanying
PR: https://github.com/RosettaCommons/pyrosetta-extras/pull/18
Add `decorator` package as a PyRosetta dependency (#746)
This PR patches a nascent bug in the `pyrosetta.distributed` required
dependencies:
`source/src/python/PyRosetta/src/pyrosetta/distributed/utility/log.py`
imports the `decorator` package, although it's not explicitly declared
as a dependency in either the unit tests or the
[`pyrosetta-distributed`](https://www.piwheels.org/project/pyrosetta-distributed/)
meta-package. Currently, the PyRosetta unit tests indirectly rely on the
`decorator` package to be installed via the `jupyter` -> `ipykernel` ->
`ipython` -> `decorator` dependency chain, and `ipython` seems to have
removed it as a dependency in their latest 9.16.0 version (see the
following PR: https://github.com/ipython/ipython/pull/15310).
Testing Pose bindings/numeric unit tests with standard PyRosetta builds (#708)
This PR splits out two tests from `T900_distributed.py` that don't
require the `pyrosetta.distributed` framework, but instead only rely on
`numpy` and ought to be tested by the standard PyRosetta builds.
---------
Co-authored-by: Sergey Lyskov <3302736+lyskov@users.noreply.github.com>
Fix off-by-one in 1-indexed loops in docking/membrane/misc (split 4/4 of #707) (#733)
## Summary
One of **four PRs splitting #707** (originally a single 35-file
off-by-one batch) into coherent, per-subsystem pieces, so each can be
reviewed and its regression-test impact assessed independently. #707 is
being closed in favor of these four. The split is deliberate — the
individual diffs are below the usual bundling threshold — because the
changes are behavior-affecting and the maintainer asked to isolate
regression-test impact per subsystem.
**This PR: docking / membrane / antibody / match / loops / misc movers
(11 files).**
### Off-by-one in 1-indexed loops (`<` → `<=`)
The last element was silently skipped:
- `docking/DockingEnsemblePrepackProtocol.cc` ×2 — chain identity
validation
- `docking/metrics.cc` ×2 — cutpoint scan in `calc_Fnonnat`
- `membrane/MPLipidAccessibility.cc` ×2 — slice iteration
- `antibody/residue_selector/CDRResidueSelector.cc`
- `match/output/UpstreamDownstreamCollisionFilter.cc`
- `loops/util.cc` ×3 — non-protein-chunk and per-loop accounting
- `cartesian/md.cc` ×2 — per-atom state save / derivative check
- `cutoutdomain/CutOutDomain.cc` — `find_nearest_res`
- `enzdes/EnzRepackMinimize.cc` — movable-residue collection for backrub
- `moves/PyMOLMover.cc` — `relevant_residues` mask init (last entry left
default `false` despite the intent of "all relevant")
### Comment-only
- `membrane/util.cc` — documents **why** a nearby loop intentionally
stays `< chains.size()` (the trailing MEM virtual chain must be
excluded). No behavior change.
Apply Rule of Zero to protocols/ classes with redundant destructors (#725)
## Summary
Applies the Rule of Zero to a set of `protocols/` classes that declared
a redundant special member — a destructor equivalent to the
implicitly-generated one — and lets the compiler generate it instead.
Each removed destructor was either `= default` or an empty `{}` body,
and in every case the class holds only **non-owning** raw pointers
(back-references it does not allocate or free), so no destructor logic
is lost and ownership semantics are unchanged.
Classes updated:
- `antibody/clusters/CDRClusterSet` — drop `= default` dtor (`ab_info_`
is a non-owning back-pointer).
- `hbnet/NetworkState` — drop empty `~NetworkState(){}` (non-owning
`HBondEdge const *`).
- `jobdist/AtomTreeDiffJobDistributor` — drop `= default` dtor
(non-owning `Pose const * last_ref_pose_`).
- `nmr/pcs/AtomGridPoint` — drop empty dtor (non-owning `Residue const
*`).
- `noesy_assign/FloatingResonance` — drop `= default` dtor (non-owning
`ResonanceList const *`).
- `noesy_assign/PeakAssignment` — drop `= default` dtor (non-owning
`CrossPeak *`).
- `peptide_deriver/PeptideDeriverBasicStreamOutputter` — drop `=
default` dtor **and** the user-declared copy constructor, which
performed a plain memberwise copy (`out_p_`, `prefix_`) identical to the
implicit one. `out_p_` is a non-owning `orstream *`.
For the classes deriving from a base with a virtual destructor
(`override` dtors), the implicitly-generated destructor is virtual and
inherited, so polymorphic destruction is unchanged. Removing these
destructors also re-enables implicit move operations (previously
suppressed), which is harmless for these value/back-pointer members. A
couple of now-moot "auto-generated destructor" doc comments were removed
alongside their declarations.
Apply Rule of Zero to empty destructors across core/ (#706)
## Summary
Remove trivially empty (`~X() {}`) or `= default;`-bodied destructors
across `core/`, **deleting the declaration outright** rather than
re-declaring it as `= default` in the header. The now-redundant
out-of-line `.cc` definitions are removed as well.
Per @roccomoretti's review, two corrections to the original approach:
1. **`~X() override = default;` is unnecessary.** For every derived
class here the base already declares a virtual destructor, so the
destructor is virtual by inheritance whether or not it is declared — the
explicit declaration bought nothing. Omitting it is the actual Rule of
Zero: it keeps the destructor virtual (inherited) and drops the
boilerplate.
2. **Defaulting the destructor does *not* restore the implicit move
members.** A `~X() = default;` destructor is still *user-declared*,
which suppresses the implicit move constructor/assignment exactly as a
user-provided one would. (An earlier version of this description wrongly
claimed defaulting "unlocks" moves.) Only omitting the destructor
entirely lets the implicit moves be generated — and then only for
classes with no other user-declared special members.
Two classes are polymorphic bases that need an explicitly-declared
virtual destructor and therefore **keep `virtual ~X() = default;`**:
`LKHBondInfo` and `HBondInfo`.
## Affected sibling groups
- `core/conformation/membrane/{AqueousPoreParameters, ImplicitLipidInfo,
MembraneGeometry, MembraneInfo}` and `membrane_geometry/{Bicelle,
DoubleVesicle, Slab, Vesicle}` — 8 classes
- `core/conformation/parametric/{Boolean, Real, RealVector, Size,
SizeVector}ValuedParameter` — 5 classes
- `core/io/silent/SilentFileData::{iterator, const_iterator}` — inline
empty dtors removed
- `core/pack/interaction_graph/RotamerDots.{hh,cc}` — `DotSphere`,
`RotamerDots`, `RotamerDotsCache`, `InvRotamerDots`
- `core/scoring/etable/EtableEnergy.{hh,cc}` — `EtableEvaluator`,
`AnalyticEtableEvaluator`, `TableLookupEvaluator`
- `core/scoring/hbonds/graph/HBondInfo.hh` — `LKHBondInfo`, `HBondInfo`
kept as `virtual ~X() = default;` (polymorphic roots); also removed an
unused user-defined empty copy ctor on `LKHBondInfo`
- `core/scoring/nmr/NMRDummySpinlabelVoxelGrid.{hh,cc}` —
`VoxelGridPoint`, `NMRDummySpinlabelAtom`, `VoxelGridPoint_AA`,
`NMRDummySpinlabelVoxelGrid`
- `core/scoring/sc/MolecularSurfaceCalculator::Atom`
- `core/energy_methods/SAXSEnergy`
Fix off-by-one in 1-indexed loops in frag_picker + accounting (split 2/4 of #707) (#731)
## Summary
One of **four PRs splitting #707** (originally a single 35-file
off-by-one batch) into coherent, per-subsystem pieces, so each can be
reviewed and its regression-test impact assessed independently. #707 is
being closed in favor of these four. The split is deliberate — the
individual diffs are below the usual bundling threshold — because the
changes are behavior-affecting and the maintainer asked to isolate
regression-test impact per subsystem.
**This PR: fragment picking + job / metric accounting (9 files).**
Enumeration/accounting loops that use `for ( i = 1; i <
container.size(); ++i )` with `i` as a direct 1-indexed accessor,
silently skipping the last element. Changed `<` to `<=`.
- `frag_picker/GrabAllCollector.hh` — `clear`
- `frag_picker/VallProvider.cc` — `find_chunk`
- `frag_picker/quota/QuotaCollector.cc` ×5 — per-position pool
enumeration
- `frag_picker/scores/AtomBasedConstraintsScore.cc` ×2 —
`constrainable_atoms` map fill, per-row state init
- `jd3/JobGenealogist.cc` — `newick_tree`
- `pose_metric_calculators/DecomposeAndReweightEnergiesCalculator.cc`
- `pose_metric_calculators/SurfaceCalculator.cc` — per-residue summary
string
- `canonical_sampling/mc_convergence_checks/HierarchicalLevel.cc` —
address match count
-
`unfolded_state_energy_calculator/UnfoldedStateEnergyCalculatorMover.cc`
— protein-residue count
## Regression tests
This PR touches **`unfolded_state_energy_calc`** (via
`UnfoldedStateEnergyCalculatorMover.cc` — the trailing residue is now
counted). That output change is intentional but results-affecting and
warrants scientific sign-off before merge. The remaining files
(frag_picker, jd3, metric calculators, `HierarchicalLevel`) are not in
the reported changed-test set.
Fix GCC 16 (trunk) debug build: real bug + dead unused-but-set-variable cleanup (#728)
## Summary
This machine has GCC 12 through GCC 16 (16 is an experimental trunk
build) installed side by side. A survey of the debug library build under
each version found:
- **GCC 12, GCC 13**: build cleanly out of the box.
- **GCC 14, GCC 15**: need the `template-id-cdtor` fix in #723.
- **GCC 16**: needs #723's fix *plus* the changes in this PR.
Building under GCC 16 (with #723's fix applied) surfaced 16 unique
`-Werror` sites — mostly `-Wunused-but-set-variable`, plus two
`-Wmaybe-uninitialized` cases (one of which is a real bug):
1. **Real bug** — `core/chemical/CacheableResidueTypeSets.cc`: the copy
constructor initialized its base class with `CacheableData(*this)`
instead of `CacheableData(other)`, reading from the not-yet-constructed
destination object rather than the fully-constructed source. Harmless
today only because `CacheableData` has no data members of its own; still
wrong and exactly what GCC 16 is right to flag.
2. **False-positive trigger** —
`protocols/simple_moves/MissingDensityToJumpMover.cc`: the default
constructor called `MissingDensityToJumpMover::get_name()` (a qualified
call through `*this`, mid-construction) to build an argument for the
`Mover` base class. `get_name()` just returns a string literal, so it's
passed directly instead — avoids the pattern rather than working around
a compiler quirk.
3. **Dead loop counters** (13 sites across 9 files) — variables
incremented alongside a real loop iterator but never read anywhere:
`EnergyGraph.hh` (`iilag`, 2 of 4 occurrences — the other two are real
array indices and are untouched), `PDBInfo.cc` (`idx`, x2),
`mmtf_writer.cc` (`chainIndex`, `modelIndex`), `md.cc` (`imap`),
`StructureDataFactory.cc` (`cur_chain`), `FoldArchitectMover.cc`
(`count`), `pose_mod.hh` (`current_pos`), `DistanceScoreMover.cc`
(`ct_peaks`), `StructureDependentPeakCalibrator.cc` (`pose_ct`). No
behavior change — removed the tracking, left the actual iteration logic
untouched.
4. **Deliberately-unused, kept** — `SapConstraintHelper.cc`'s `offset`
is tracked "for symmetry" per an existing comment even though never
read. Rather than removing it against that stated intent, added an
explicit `(void)offset;` cast to satisfy the warning.
Fix Slab geometry thickness/steepness argument order in MembraneInfo (#719) (#722)
## Summary
Fixes #719. `Slab`'s constructor is `Slab(core::Real steepness,
core::Real thickness)`, but two `MembraneInfo` constructors in
`core/conformation/membrane/MembraneInfo.cc` called it with the
arguments in the wrong order.
**(1) Plain 6-arg constructor** passed `Slab(thickness, steepness)`, so
the `Slab` geometry stored thickness and steepness swapped. The
`fa_2012` energy methods (`FaMPEnv`/`FaMPSolv` →
`mpframework_smooth_fa_2012`) read thickness/steepness off the geometry,
so this silently changed scores (e.g. with `thickness=15.6,
steepness=10`, the IMM1 transition midpoint became 10 Å instead of 15.6
Å with a far steeper transition). Now passes `Slab(steepness,
thickness)`.
**(2) Implicit-lipid constructor** passed `Slab(thickness, steepness)`,
where the bare `thickness` token was *not* the member `thickness_` (just
set from `implicit_lipids_->water_thickness()`) but the unscoped
`MEM::thickness` enum (`== 1`) from `MembraneParams.hh` — both live in
`core::conformation::membrane`. The call was therefore effectively
`Slab(1, steepness)`: the lipid water thickness was dropped and `1`
landed in the steepness slot. Now passes `Slab(steepness, thickness_)`,
matching the sibling lipid + `MP_GEOMETRY_TRANSITION` constructor's
`SLAB` case. This path is currently latent (these constructors set
`implicit_lipids_`, so `use_franklin()` is true and the geometry
thickness/steepness aren't read), but it was still wrong.
All other `Slab` call sites in the file already use the correct
`(steepness, thickness)` / `(steepness, thickness_)` order, so these two
lines are the complete set of violations.
Diagnosis and reproduction by @vuoanh in #719.