Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue with GRID_MIN/GRID_MAX in HISTOGRAM action #717

Closed
valsson opened this issue Jul 14, 2021 · 6 comments · Fixed by #722
Closed

Issue with GRID_MIN/GRID_MAX in HISTOGRAM action #717

valsson opened this issue Jul 14, 2021 · 6 comments · Fixed by #722
Assignees

Comments

@valsson
Copy link
Contributor

valsson commented Jul 14, 2021

By chance, I noticed an issue with GRID_MIN/GRID_MAX in HISTOGRAM action.

It is possible to give a random string as a value to GRID_MIN/GRID_MAX and the code still runs without an error. Instead, it seems to take 0.0 as the grid value.

For example, you can see this in regtest/analysis/rt-histo:

If you use

HISTOGRAM ...
ARG=x
GRID_MIN=0.0
GRID_MAX=blabla
GRID_BIN=ffa
BANDWIDTH=0.1
LABEL=hA1
... HISTOGRAM

Gives

#! FIELDS x hA1 dhA1_x
#! SET normalisation 1.0000
#! SET min_x 0.0
#! SET max_x blabla
#! SET nbins_x 100
#! SET periodic_x false
0.0000 1.0000 0.0000
0.0000 0.0000 0.0000
0.0000 0.0000 0.0000
0.0000 0.0000 0.0000
0.0000 0.0000 0.0000
0.0000 0.0000 0.0000
0.0000 0.0000 0.0000
0.0000 0.0000 0.0000
0.0000 0.0000 0.0000
0.0000 0.0000 0.0000
0.0000 0.0000 0.0000
0.0000 0.0000 0.0000
0.0000 0.0000 0.0000
0.0000 0.0000 0.0000
0.0000 0.0000 0.0000
0.0000 0.0000 0.0000
0.0000 0.0000 0.0000

I guess it would be better to add a check on either the parsing of GRID_MIN/GRID_MAX that the parameters gives are proper numbers. Or maybe better in the setBounds function in gridtools/HistogramOnGrid.cpp to avoid this problem happening elsewhere.

@valsson
Copy link
Contributor Author

valsson commented Jul 14, 2021

I note that using

GRID_MIN=-pi
GRID_MAX=pi

Works as expected

@GiovanniBussi
Copy link
Member

@gtribello could it be related to the fact that these conversions do not check the returned bool?

@gtribello
Copy link
Member

@GiovanniBussi yes I think if you check the returned bool and put in an error then the problem will be fixed.

@GiovanniBussi
Copy link
Member

What scares me is the output of this command:

git grep "Tools::convert"

@GiovanniBussi
Copy link
Member

In many cases it is used to convert numbers to strings (e.g. for printing) and that's fine. But when it is used for reading an input it could have this problem.

@GiovanniBussi
Copy link
Member

I confirm (in regtest/basic/rt21/plumed.dat) that if I change:

m1g: METAD ARG=r1g HEIGHT=0.1 PACE=10000000 SIGMA=1.0  FILE=H1G GRID_MIN=0.0 GRID_MAX=20.0 GRID_BIN=100

to

m1g: METAD ARG=r1g HEIGHT=0.1 PACE=10000000 SIGMA=1.0  FILE=H1G GRID_MIN=bla GRID_MAX=20.0 GRID_BIN=100

The result is not affected. This is actually very annoying... Thanks @valsson for pointing this out!

@GiovanniBussi GiovanniBussi self-assigned this Jul 18, 2021
GiovanniBussi added a commit that referenced this issue Jul 20, 2021
In the original fix I was making convert() throw exceptions.
However, we use a lot failed conversions when reading inputs.
Since catching an exception is much more expensive than inspecting
a returned bool, this was slowing down a lot some parts of the code

In 0ed75e9 I solved the issue
by allowing to throw only a few functions.

Here I take a radically different approach: a new function (convertNoexcept)
can be used like to old one, and returns a bool. The function
convert just calls convertNoexcept and, if false, throws.

In this manner there is no overhead expected at all with respect to the
previous version of the code.

In addition, the modifications are less intrusive. It is sufficient to
replace convert with convertNoexcept in all the places where the returned
bool was checked
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants