Repository navigation
Conversation
# Conflicts: # namd/src/colvarproxy_namd.h # src/colvarproxy_system.cpp
Register backend alpha limits during component initialization, before grid parameters and hard-boundary dependencies are parsed. Propagate initialization errors and preserve explicit boundary conflict checks. Add regression coverage for omitted and matching boundaries, conflict diagnostics, hard-boundary defaults and overrides, transformed components, non-TABF bounds, and backend query failures.
Use module-owned errors for unsupported NAMD TABF operations. Preserve non-finite checks under fast-math and guard UI input records against invalid allocated-bin indices. Add reflection and UI regression tests, including production fast-math coverage, and match reflection test compilation settings to optional library features such as Lepton.
Accept NAMD fullElectFrequency 0 as an inactive schedule while retaining the every-step requirement for active full electrostatics. Clarify the diagnostic without changing NAMD core scheduling checks.
|
@jhenin @giacomofiorin @HanatoK, could you please review the updated TABF changes and help resolve the CI failures with Apptainer installation and the ARM64 container format? Thank you! |
You are missing updates from the latest |
|
Separately, @fhh2626, can you adapt (at least in part) the PR template and fill the relevant information, e.g. regarding backends affected (just NAMD or others?) and the AI usage? |
|
Looks good to me overall. However, both this PR and https://gitlab.com/tcbgUIUC/namd/-/merge_requests/537 do not document the new options and CV |
SummaryThis PR adds The follow-up changes address the review comments from #949:
An existing loop-index bug in the UI estimator was also fixed, with explanatory comments added to the code. TABF alpha remains unsupported by the UI and CZAR estimators. Requests for these combinations are explicitly rejected. Related issue(s)This PR supersedes #949. The original review discussion remains available there. The corresponding NAMD implementation is: Affected engine(s)
The latest master has been merged into this branch, including its updated CI workflows. Author(s)LicensingThe code contributed in this PR follows the Colvars project's license, the GNU Lesser General Public License version 3 (LGPLv3). Artificial Intelligence (AI)All code contributed in this PR was written using AI tools. It was reviewed separately by models from at least two different AI providers, including a GPT-series model. Further information / linksRelated paper: DocumentationI will submit a separate PR to update the Colvars documentation for |
giacomofiorin
left a comment
There was a problem hiding this comment.
From a general point of view, this looks similar to how the alchLambda feature is already implemented, which made review a bit easier. However, this is highly dependent on the NAMD MR, so I think it should be revisited when that is merged.
Besides a few specific comments, I think these are significant items missing:
- A regression test using NAMD, preferably under
namd/tests/interface - LaTeX documentation; the reference to your paper could be added to
doc/colvars-code-refs.bib, annotated in the same way as the existing references so that the internal Colvars citation reporter can pick it up - A clear error message when folks try (and fail) to use the feature outside NAMD (especially in VMD, which is a companion code to NAMD)
| int colvarproxy_system::get_tabf_alpha(cvm::real * /* alpha */) | ||
| { | ||
| return cvm::error_static("Error in get_tabf_alpha: TABF alpha dynamics is not supported by this build.", | ||
| COLVARS_NOT_IMPLEMENTED); |
There was a problem hiding this comment.
This and other functions are using cvm::error_static() only because their class does not own a cvmodule pointer. But these functions are also only called from the colvar class, which does have such a pointer. So you could simply return the integer value COLVARS_NOT_IMPLEMENTED and call cvmodule->error() from the colvar class.
| provide(f_cvc_Jacobian); // Zero | ||
|
|
||
| x.type(colvarvalue::type_scalar); | ||
|
|
There was a problem hiding this comment.
One can use init_scalar_boundaries() to set predefined values of the boundaries (which we also forgot to use it in alchLambda).
| return cvcs; | ||
| } | ||
|
|
||
| /// Whether this colvar has a component of the given function type |
There was a problem hiding this comment.
"at least one" component?
|
|
||
| // Detect if we have a single component that is an alchemical lambda | ||
| const bool is_tabf_alpha = | ||
| is_enabled(f_cv_single_cvc) && cvcs[0]->function_type() == "tabfAlpha"; |
There was a problem hiding this comment.
This should really be a separate function that init() would call if the condition is true. We had historically very long member functions, but we have broken them up quite a bit recently, and the AI agent should look into that as well, or at least balance good practices with consistency with the existing code.
| cvm::real tabf_lower = 0.0; | ||
| cvm::real tabf_upper = 0.0; | ||
| error_code |= cvmodule->proxy->get_tabf_alpha_boundaries(&tabf_lower, &tabf_upper); | ||
| const bool lower_was_set = key_lookup(conf, "lowerBoundary"); |
There was a problem hiding this comment.
There is a feature called f_cv_lower_boundary to test for this: if (is_enabled(f_cv_lower_boundary)) { ... }. Is there a reason for not using that instead of parsing the input again?
|
|
||
| if (uses_tabf_alpha) { | ||
| bool requested_UI_estimator = false; | ||
| get_keyval(conf, "UIestimator", requested_UI_estimator, false, colvarparse::parse_silent); |
| namespace { | ||
| // Floating-point classification may be folded to true under -ffast-math. | ||
| // Inspect the IEEE binary64 exponent instead, without type-punning. Supported | ||
| // targets use the same byte order for double and uint64_t (including x86/ARM); | ||
| // mixed-endian floating-point representations are not supported by this helper. | ||
| bool reflection_is_finite(cvm::real value) | ||
| { | ||
| #if defined(__FLOAT_WORD_ORDER__) && defined(__BYTE_ORDER__) | ||
| static_assert(__FLOAT_WORD_ORDER__ == __BYTE_ORDER__, | ||
| "Reflection does not support mixed-endian floating point"); | ||
| #endif | ||
| static_assert(sizeof(cvm::real) == sizeof(std::uint64_t) && | ||
| std::numeric_limits<cvm::real>::is_iec559 && | ||
| std::numeric_limits<cvm::real>::radix == 2 && | ||
| std::numeric_limits<cvm::real>::digits == 53 && | ||
| std::numeric_limits<cvm::real>::max_exponent == 1024, | ||
| "Reflection requires IEEE 754 binary64 cvm::real"); | ||
| std::uint64_t bits; | ||
| std::memcpy(&bits, &value, sizeof(bits)); | ||
| // LLVM can recognize even memcpy + exponent masking as floating-point | ||
| // classification and fold it under finite-math assumptions. Observe the | ||
| // representation through a volatile integer to prevent that transformation. | ||
| volatile std::uint64_t observed_bits = bits; | ||
| return (observed_bits & UINT64_C(0x7ff0000000000000)) != UINT64_C(0x7ff0000000000000); | ||
| } | ||
| } |
There was a problem hiding this comment.
Can the AI agent clarify if this would work on all compilers including MSVC?
| virtual void calc_force_invgrads(); | ||
| virtual void calc_Jacobian_derivative(); | ||
| virtual void apply_force(colvarvalue const &force); | ||
| virtual int set_external_value(bool now=false); |
There was a problem hiding this comment.
Probably best to skip the virtual if this is a specialized function, with an optional parameter on top of that
PR migration
This draft PR supersedes #949, using the
tabfbranch in the main Colvars repository as suggested by @jhenin and @giacomofiorin, so that backend CI can run without the fork-related configuration restrictions.The implementation is unchanged by this PR migration. The original discussion and review comments remain available in #949; I will address the outstanding items here before requesting another review.
Outstanding review items
cvm::error_staticcalls in the NAMD TABF proxy withcvmodule->error, preserving the unsupported-build error semantics, and check the analogous TABF methods for consistency.std::isfinite/-ffast-mathcompatibility concern in the reflecting-boundary implementation.Original implementation description
This PR implements the use of the temperature scaling factor α as a CV (https://doi.org/10.1021/acs.accounts.5c00666), achieving REMD-like sampling with smaller temperature intervals. A new version of NAMD is required (https://gitlab.com/tcbgUIUC/namd/-/merge_requests/537).
example:
In NAMD config: