Colvars/colvars

Better error message when trying to load coordinates from missing files

giacomofiorin opened this issue · 3 comments

When loading a file that isn't XYZ the first branch of the conditional below fails, falling back to the PDB reader:

colvars/src/colvarmodule.cpp

Lines 2159 to 2171 in 8eb35e7

if (colvarparse::to_lower_cppstr(ext) == std::string(".xyz")) {
if (pdb_field.size() > 0) {
return cvm::error("Error: PDB column may not be specified "
"for XYZ coordinate files.\n", COLVARS_INPUT_ERROR);
}
// For XYZ files, use internal parser
error_code |= cvm::main()->load_coords_xyz(file_name, &sorted_pos, atoms);
} else {
// Otherwise, call proxy function for PDB
error_code |= proxy->load_coords(file_name,
sorted_pos, atoms->sorted_ids(),
pdb_field, pdb_field_value);
}

In GROMACS the resulting error message can be confusing.

It may be clearer to check first if the file is present and readable before discussing its format.

Couldn't the same purpose be achieved by a more portable test? Namely try to open the file, and if not good() we give up.

Second question: will this play nicely with the input file caching in GROMACS? At mdrun time, the input files may not exist in the filesystem.

Couldn't the same purpose be achieved by a more portable test? Namely try to open the file, and if not good() we give up.

Second question: will this play nicely with the input file caching in GROMACS? At mdrun time, the input files may not exist in the filesystem.

Both good points. In that case, rather than reinventing the wheel or making the tests more complex for GROMACS, we could perhaps recognize that load_coords() is really intended for PDB files, and make its error messages more explicit that way.

This would also help in the case where people try using a PDB file in NAMD or VMD and then suddenly it doesn't work in GROMACS...

Both good points. In that case, rather than reinventing the wheel or making the tests more complex for GROMACS, we could perhaps recognize that load_coords() is really intended for PDB files, and make its error messages more explicit that way.

Implemented in #673.