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.
Drop template-id from ctor/dtor names in numeric/ (GCC 14/15 -Werror=template-id-cdtor) (#723)
## Summary
GCC 14+ enforces `-Werror=template-id-cdtor`: a class template may not
name its own constructors or destructors with explicit template
arguments — the injected-class-name must be used without `<...>`.
`numeric/MathVector.hh`, `numeric/MathMatrix.hh`, and
`numeric/histograms/OneDHistogram.hh` declared constructors (and, for
the first two, the destructor) in the disallowed form, e.g.:
```cpp
MathVector< T>() : ...
explicit MathVector< T>( const Size SIZE, ... ) : ...
~MathVector< T>() { ... }
OneDHistogram<key1>()= default;
```
Under GCC 14/15 in C++20 mode this fails to compile (`error: template-id
not allowed for constructor/destructor in C++20`). The `numeric/`
occurrences broke the plain library debug build before it could even
reach `basic/` or `core/`. The `OneDHistogram.hh` occurrence is more
subtle: its default constructor is only instantiated by a unit test
(`numeric/histograms/OneDHistogram.cxxtest.hh`), not by any library
code, so it slipped past a library-only build and only surfaced when
building/running the unit test suite — this is what broke CI on the
previous version of this PR.
This drops the `< T>` / `<key1>` from the constructor and destructor
declarator-ids, making them consistent with the copy constructors in the
same classes, which already use the correct injected-class-name form.
Return types, operators, and `new` expressions that legitimately use the
templated name are untouched. Pure syntactic correction, no semantic
change.
Verified with a clean `mode=debug` library build **and** `mode=debug
cat=test` unit-test build under GCC 15 (this machine's default), plus a
full library build under GCC 14 — both fully green. A targeted scan of
`source/src`, `source/test`, and `source/src/devel` for the same
declaration pattern turned up no other occurrences.
## Relationship to existing GCC 15 PRs (#555, #556)
This is not the first attempt at the GCC 15 build. Two earlier community
PRs are still open and overlap with this one:
- **#555 — "Fix C++20 template-id errors in constructors/destructors"**
(@saberger). This contains the **identical**
MathVector/MathMatrix/OneDHistogram template-id fix as this PR, and
additionally covers `core/scoring/lkball/LK_DomeEnergy.cc`,
`protocols/forge/remodel/RemodelGlobalFrame.cc`,
`utility/options/VectorOption_T_.hh`, two unit tests, and
`tools/build/basic.settings` — none of which have this same
template-id-cdtor pattern currently (checked directly), so those are
addressing separate GCC 15 issues.
- **#556 — "Fixes for compiling with gcc15"** (@roccomoretti). The
broader assorted GCC 15 fixes. Does not touch these three files.
Because #555 already lands the same fix, #723 is largely redundant with
the `numeric/` slice of that effort. It's offered as a minimal,
narrowly-scoped version of just the `template-id-cdtor` correction in
case it's useful to merge the build-blocking part independently;
otherwise #555 (combined with #556, per its author's note) supersedes
it. Closing this in favor of #555 + #556 is fine if the maintainers
prefer to consolidate the GCC 15 work.
## Note on diff size
This is a small change (three files, ~18 lines) — below the usual
bundling threshold — but it is the complete fix for this pattern across
the codebase: a scan of `source/src`, `source/test`, and
`source/src/devel` found no other occurrences of a class template naming
its own constructor/destructor with a template-id. Kept narrowly scoped
to this one toolchain-compatibility pattern.