Fix #569 and allow mixing scalar and vector CVCs

[HanatoK]

This PR tries to fix #569. In addition, I think it is time to remove the constraint of using different types of nested CVCs.

[zwpku]

I am not very sure about the exact requirement on the sub-cvcs of linearcombination (and its subclasses). It seems that the function colvar::linearCombination::calc_value() actually requires sub-cvcs are either all scalars or all vectors of the same length(?), since it contains the following lines:

if (current_cv_value.type() == colvarvalue::type_scalar) {
    x += cv[i_cv]->sup_coeff * (cvm::pow(current_cv_value.real_value, cv[i_cv]->sup_np));
} else {
    x += cv[i_cv]->sup_coeff * current_cv_value;
}

[giacomofiorin]

@HanatoK Ok with the quick fix for a clear bug but I don't know how to assess the removal of the same-type constraint, given how general that CVC is. What are the conditions that made you add the constraint in the first place?

[HanatoK]

I am not very sure about the exact requirement on the sub-cvcs of linearcombination (and its subclasses). It seems that the function colvar::linearCombination::calc_value() actually requires sub-cvcs are either all scalars or all vectors of the same length(?), since it contains the following lines:

if (current_cv_value.type() == colvarvalue::type_scalar) {
    x += cv[i_cv]->sup_coeff * (cvm::pow(current_cv_value.real_value, cv[i_cv]->sup_np));
} else {
    x += cv[i_cv]->sup_coeff * current_cv_value;
}

Yes. In the linearCombination class if the types are different then there will be an error raised in calc_value(). I remove that requirement in the constructor, so that the derived classes such as torchann and customColvar can use something like

colvar {
    name nn_0
    width 0.05
    extendedLagrangian on
    extendedFluctuation 0.1
    extendedTimeConstant 200
    customColvar {
        name z
        customFunction x+y2
        dihedral {
            group1 {atomNumbers {25}}
            group2 {atomNumbers {27}}
            group3 {atomNumbers {29}}
            group4 {atomNumbers {35}}
        }
        cartesian {
            name y
            atoms {
                atomNumbers {35}
            }
        }
    }
}

which is not possible before this commit.

[HanatoK]

@HanatoK Ok with the quick fix for a clear bug but I don't know how to assess the removal of the same-type constraint, given how general that CVC is. What are the conditions that made you add the constraint in the first place?

linearCombination is used for both (i) a real CV that can be used from the user side, and (ii) a base class for customColvar and possibly torchann in the PR of @zwpku . When I wrote the code ago I only thought (i), and now it seems an NN may accept a mixture of different types of CVCs, so it would be better to remove the constraint. As for using linearCombination with different types, the following error will occur in calc_value() (derived classes will override it):

colvars:     Trying to perform an operation between two colvar values with different types, "scalar number" and "n-dimensional vector"

[zwpku]

@HanatoK Ok with the quick fix for a clear bug but I don't know how to assess the removal of the same-type constraint, given how general that CVC is. What are the conditions that made you add the constraint in the first place?

linearCombination is used for both (i) a real CV that can be used from the user side, and (ii) a base class for customColvar and possibly torchann in the PR of @zwpku . When I wrote the code ago I only thought (i), and now it seems an NN may accept a mixture of different types of CVCs, so it would be better to remove the constraint. As for using linearCombination with different types, the following error will occur in calc_value() (derived classes will override it):

colvars:     Trying to perform an operation between two colvar values with different types, "scalar number" and "n-dimensional vector"

My comments with a limited understanding on the details:

I feel it is actually good to have some checks to guarantee that calc_value() and other member functions of linearCombination do not break down. If I understood correctly, the current constructor checks the types of sub-cvcs but not sizes if they are vectors. Function valc_value() may go wrong when the cvcs are vectors with different lengths?

For the torchann subclass, I can only speak for myself. For the moment I am satisfied to be able to define cvcs using Cartesian coordinates (could be several cartesian cvcs with different number of atoms, since the sizes are not checked) or linear combinations of scalars, e.g. angles and bonds. The code should work when cvcs have different types, but it is not clear to me how useful this feature is in applications...

[HanatoK]

When performing += for colvarvalue objects in calc_value(), the types of two operands are actually checked:

colvars/src/colvarvalue.h

Lines 527 to 529 in 110b08f

	inline void colvarvalue::operator += (colvarvalue const &x)
	{
	colvarvalue::check_types(*this, x);

Moreover, this check is more robust as it compares the sizes of two vectors if the types are vector:

colvars/src/colvarvalue.h

Lines 449 to 457 in 110b08f

	if (x1.type() == type_vector) {
	if (x1.vector1d_value.size() != x2.vector1d_value.size()) {
	cvm::error("Trying to perform an operation between two vector colvar "
	"values with different sizes, "+
	cvm::to_str(x1.vector1d_value.size())+
	" and "+
	cvm::to_str(x2.vector1d_value.size())+
	".\n");
	return COLVARS_ERROR;

So I think since the operands are checked in calc_value(), there is no need to check them again in the constructor.

[jhenin]

Looks good! It seems the CodeQL check fails because the branch comes from another repo?

Uploading results
  Processing sarif files: ["/home/runner/work/colvars/results/cpp.sarif"]
  Uploading results
  Error: ref 'refs/heads/fix_linear_combination' not found in this repository