Several improvements to colvar component classes by giacomofiorin · Pull Request #643 · Colvars/colvars

giacomofiorin · 2023-12-19T18:00:53Z

This PR contains several improvements to colvar::cvc and its derived classes, including substantial simplification of the code and some fixes where the previous implementation was incorrect (distanceVec metrics) or less robust (period).

There is now only one type of CVC constructor, which takes no arguments: this removes the need to maintain two different types of constructor. init(conf) now initializes an object from the configuration string, by modifying existing defaults.
Default values for plain old data are set right where the member is declared in the header, at long last! :-)
(Fix) The distanceVec metric functions are now consistent with the documentation.
There is no longer a need to explicitly derive the metric functions: the base class assumes a scalar-valued function, and only special cases are overridden. A ton of duplicated functions were removed with this.
period can now be set only for those CVCs that allow it: that includes distanceZ as well all periodic angles (only where someone needs something else than 360°).
std::shared_ptr is now used to collect the CVC objects inside the colvar level. I have not yet touched the arrays of CVC pointers inside complex CVC objects (e.g. the protein CVCs or the combination ones) because those will be modified anyway in upcoming work.

Other possible improvements that were out of scope to do:

Because all CVCs have now a derived init() function, it would be theoretically possible to call that from colvar::update_cvc_config(), i.e. the backend for the modifycvcs scripting command. Currently, the base class method cvc::init() is still called, because not all the derived ones are idempotent yet.
Atom groups could now be tracked much better with smart pointers, allowing them to be shared to save data/computation (this is done in Allow reusing computation and data between CVC objects #644).
colvarcomp.h could be split up to reduce interdependencies.
Feature objects could definitely use some smart-pointer love, too.

HanatoK

Some of the original constructor code with cvm::error return as early as possible, because continue to execute the code following cvm::error will result in a crash. When moving to an init function this behavior should keep the same. Otherwise VMD users may experience crashes after loading the wrong data into some of the CVs, for example, a pathFile with the wrong format for gpathCV.

HanatoK · 2024-03-08T22:09:48Z

src/colvarcomp_gpath.cpp

    if (total_reference_frames < 2) {
-        cvm::error("Error: you have specified " + cvm::to_str(total_reference_frames) + " reference frames, but gzpath requires at least 2 frames.\n");
-        return;
+        error_code |= cvm::error("Error: you have specified " + cvm::to_str(total_reference_frames) + " reference frames, but gzpath requires at least 2 frames.\n", COLVARS_INPUT_ERROR);


Why is the return statement moved to the end of the function? I am a bit worried that if it doesn't return here, the following code would segfault or crash, which is OK for NAMD but not a good user experience for VMD.

Ok with me to return early, I just overlooked the possibility of a crash.

Changed the instances that you pointed out into an early return with error.

HanatoK · 2024-03-08T22:13:12Z

src/colvarcomp_gpath.cpp

+
+int colvar::gspath::init(std::string const &conf)
+{
+    int error_code = CartesianBasedPath::init(conf);


Maybe we need to check the error code and return early too? If you think it is too complicated to return error code in derived classes, we can throw exceptions and catch them in some of the upper level functions.

Dude, are you really advocating for supporting VMD and and propose adding exceptions in the same review round? ;-)

Sorry, I should be clearer. Exception is just one of the solutions, and if I were you I might use exceptions in any init functions and catch them in colvar::update_cvc_config. Early return is another one, which may need more lines of code for both base and derived classes. Using either of them should be enough. As for VMD, it is just an example of interactive applications. Colvars as a library should not terminate the main interactive program if there is an error.

I should be clearer, too :-) Exceptions would be convenient on their own, actually. We haven't used them in the past mostly to play it safe with VMD, which traditionally aims to support exotic and/or ancient compilers. Although compiler support has become more uniform with C++11, best practices like the Google Style Guide don't encourage vendors to support them well.

I think that adding early returns wherever it looks like the code may crash is probably the safest right now. I think that's still a minority of all error conditions.

This is a common pattern in several places, I made similar "early return" changes in #650 and Haochuan added a bunch more in this PR.
I wouldn't mind having this in more places, that is, everywhere, and I wonder if there is a more concise / elegant / modern / systematic way of formulating this, besides exceptions. The C programmer in me would make it a macro... There, I said it, I'm old. C++11 best practices and Google Style Guide: get off my lawn.

Just to be clear, I'm old too but I would be happy to embrace anything that the maintainers of a major package (whatever their age might be) will commit to approve.

In writing, please.

Also remove the preprocessor macro defining member functions, in favor of using C++ inheritance throughout. Therefore, the decrease in the amount of duplicated code is much higher than how it appears from the diff.

…riables

In that code version, distanceVec was not yet honoring forceNoPBC

…ction

…code

Return the error code earlier to prevent crashes in interactive programs.

giacomofiorin · 2024-03-12T18:08:35Z

@HanatoK Thanks for revising the PCV constructors!

src/colvarcomp.cpp

jhenin · 2024-03-12T19:44:15Z

src/colvarcomp_gpath.cpp

+
+int colvar::gspath::init(std::string const &conf)
+{
+    int error_code = CartesianBasedPath::init(conf);


This is a common pattern in several places, I made similar "early return" changes in #650 and Haochuan added a bunch more in this PR.
I wouldn't mind having this in more places, that is, everywhere, and I wonder if there is a more concise / elegant / modern / systematic way of formulating this, besides exceptions. The C programmer in me would make it a macro... There, I said it, I'm old. C++11 best practices and Google Style Guide: get off my lawn.

[update-doc]

silence uninitialized access valgrind warning lammps/lammps@c3cc497

giacomofiorin mentioned this pull request Dec 19, 2023

enable use of CVs defined by PyTorch neural network models #570

Merged

giacomofiorin added stability maintenance No user-visible effects labels Dec 19, 2023

giacomofiorin removed the maintenance No user-visible effects label Jan 3, 2024

giacomofiorin mentioned this pull request Feb 11, 2024

Allow empty string as cvc config #658

Merged

giacomofiorin force-pushed the cvc-default-constructors branch 2 times, most recently from 43da335 to b705999 Compare February 20, 2024 19:18

giacomofiorin marked this pull request as ready for review February 20, 2024 19:19

giacomofiorin force-pushed the cvc-default-constructors branch from b705999 to be5a4b8 Compare February 22, 2024 12:28

giacomofiorin requested review from HanatoK and jhenin March 8, 2024 18:54

HanatoK requested changes Mar 8, 2024

View reviewed changes

giacomofiorin requested a review from HanatoK March 11, 2024 22:52

giacomofiorin force-pushed the cvc-default-constructors branch from 1e29e66 to f8a8b6c Compare March 11, 2024 22:53

giacomofiorin and others added 14 commits March 12, 2024 14:04

Separate creation and initialization of colvar components

3faab97

Update inheritance metadata

85e7a40

List forceNoPBC as an option for all components in the doc

070773b

Honor the value of forceNoPBC when comparing values of distanceVec va…

504d5bd

…riables

Remove flag from test input that was absent in reference run

a039cd8

In that code version, distanceVec was not yet honoring forceNoPBC

Condense most implementations of cvc::apply_force() into a single fun…

ce3fb4e

…ction

Remove some member functions from public scope

1e80955

Initialize some plain old data in header

728a002

Use shared pointers to handle the components of one colvar

139ee35

Integrate periodicity check in cvc::init()

415ea65

Appease compiler warning

e4d30ce

Return early if critical errors are raised in path CV initialization …

ccdd474

…code

Make some CVs (PCVs and NN-based CVs) more robust.

3957f1e

Return the error code earlier to prevent crashes in interactive programs.

giacomofiorin force-pushed the cvc-default-constructors branch from 0a6b87f to 3957f1e Compare March 12, 2024 18:04

HanatoK approved these changes Mar 12, 2024

View reviewed changes

jhenin reviewed Mar 12, 2024

View reviewed changes

jhenin self-requested a review March 12, 2024 20:35

jhenin approved these changes Mar 12, 2024

View reviewed changes

Doc formatting minor fixes

a196302

[update-doc]

giacomofiorin merged commit ec69db1 into master Mar 12, 2024

giacomofiorin deleted the cvc-default-constructors branch March 12, 2024 20:56

giacomofiorin mentioned this pull request Mar 14, 2024

Stop initializing cvs after atom group init fail #674

Merged

giacomofiorin referenced this pull request Mar 28, 2024

Backport small fix from LAMMPS

5b3b24f

silence uninitialized access valgrind warning lammps/lammps@c3cc497

giacomofiorin mentioned this pull request Aug 5, 2024

Colvars update lammps/lammps#4261

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Several improvements to colvar component classes#643

Several improvements to colvar component classes#643
giacomofiorin merged 15 commits intomasterfrom
cvc-default-constructors

giacomofiorin commented Dec 19, 2023 •

edited

Loading

Uh oh!

HanatoK left a comment

Uh oh!

HanatoK Mar 8, 2024

Uh oh!

giacomofiorin Mar 8, 2024

Uh oh!

giacomofiorin Mar 8, 2024

Uh oh!

HanatoK Mar 8, 2024

Uh oh!

giacomofiorin Mar 8, 2024

Uh oh!

HanatoK Mar 8, 2024

Uh oh!

giacomofiorin Mar 8, 2024

Uh oh!

jhenin Mar 12, 2024

Uh oh!

giacomofiorin Mar 12, 2024

Uh oh!

giacomofiorin commented Mar 12, 2024

Uh oh!

Uh oh!

jhenin Mar 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

giacomofiorin commented Dec 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HanatoK left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giacomofiorin commented Mar 12, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

giacomofiorin commented Dec 19, 2023 •

edited

Loading