Colvars / colvars

Collective variables library for molecular simulation and analysis programs
http://colvars.github.io/
GNU Lesser General Public License v3.0
209 stars 57 forks source link

"cv load" should not active colvar value check #187

Closed fhh2626 closed 4 years ago

fhh2626 commented 6 years ago

Hi, all,

I would like to suggest that "cv load" should not active: if (after_restart) { if (cvm::proxy->simulation_running()) { cvm::real const jump2 = dist2(x, x_restart) / (width*width); if (jump2 > 0.25) { cvm::error("Error: the calculated value of colvar \""+name+ "\":\n"+cvm::to_str(x)+"\n differs greatly from the value " "last read from the state file:\n"+cvm::to_str(x_restart)+ "\nPossible causes are changes in configuration, " "wrong state file, or how PBC wrapping is handled.\n", INPUT_ERROR); return INPUT_ERROR; } } in colvar.cpp. Since when one wants to use "cv load", the system's configuration has usually changed.

Thanks, Haohao

giacomofiorin commented 6 years ago

@fhh2626 I most likely would not recommend skipping this check every time that cv load is used, as some people prefer using it in scripts instead of colvarsInput, which is not a Tcl command. Furthermore, cv load is the only option available if you are reinitializing Colvars alongside NAMD's reinitatoms.

Can you clarify the use case where you want to load a Colvars state file that is inconsistent with the current Cartesian coordinates?

fhh2626 commented 6 years ago

@giacomofiorin I described the case in #186 . Maybe some interface like "cv load xxx false" to skip the check?

jhenin commented 6 years ago

So here you want to load the state of the biases, but not the colvars. This is a kind of partial restart situation.

I wonder if this error condition could be reduced to a warning. I know warnings are easy to ignore, but this is a case of trying to clean up after user mistakes, that can't be expected everywhere.

The only reasonable alternative I can see is exposing a flag in the interface, as proposed by Haohao.

giacomofiorin commented 6 years ago

Given the explanation, I think that a selective load method for the variables and biases will cover this case well. This will be implemented by the improved scripting interface that I'm working on.

@fhh2626 Since NAMD 2.13 is now closed, can you disable the check in your local branch or in a feature branch of this repository, and work with that version? You could also implement this selective load method to merge into master: it would be replaced by my function, but the functionality would be identical: letting an individual colvar or colvarbias object load only its own portion of a state file, ignoring the rest and returning with an error only if the state is not found.

fhh2626 commented 6 years ago

Thanks for you explanation @giacomofiorin . I will disable this check first and see if I can do something on the selective load.

jhenin commented 4 years ago

Right now the restarts are read based on the order of objects. Could they use their names instead? That would seem more robust. (More of a question for @giacomofiorin )

giacomofiorin commented 4 years ago

Ordinarily when you add a new variable or bias and also need to restart, the safe bet is to (1) load the old configuration, (2) load the state file, and (3) define the new objects. It may require to use a slightly different script after the state file begins containing the new objects: but that's kind of fair since a change in protocol midway through a run is not a trivial one.

As for deleting a bias, restraints can be turned off and metadynamics or ABF can be told to stop updating (deleting them altogether is an edge case).

So there haven't been many occasions where handling the restart after changing the config has been a risky or difficult scenario.

That said, I'm happy to take a look at this again (unless Haohao already has some progress on this issue, of course).

On Fri, Nov 22, 2019, 09:19 Jérôme Hénin notifications@gitsub.com wrote:

Right now the restarts are read based on the order of objects. Could they use their names instead? That would seem more robust. (More of a question for @giacomofiorin https://github.com/giacomofiorin )

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Colvars/colvars/issues/187?email_source=notifications&email_token=ABCNSWMKOXKPXK3RMRGJ3A3QU7S6PA5CNFSM4F76GESKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEE5YLNY#issuecomment-557548983, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABCNSWONCVSPFZUXFGEROGLQU7S6PANCNFSM4F76GESA .

jhenin commented 4 years ago

I have another use case where a partial restart is necessary, with some objects from the previous run being absent from the current one. It would be very cumbersome to carry over the information neede to redefine the objects we don't want anymore, just to delete them immediately after loading their state. It would make the generated colvars config twice as long, with useful information hidden among useless stuff.

For now I have scripts that run sed on the restart files to remove the unneeded blocks. That is of course not very satisfactory.

My current view of the ideal restarting process would be:

Any comments on whether this is a) desirable, and b) feasible?

giacomofiorin commented 4 years ago

That's a legitimate need, but the i/o can get very inefficient if the same stream is looped over multiple times.

Ideally, there should be a table of offsets that allows to jump (via seekg()) to the relevant offset in the state file.

It's safe to say that these offsets won't be known until the whole file is written, so they would have to be written to another file, or at the top of the state file itself using a non-sequential write.

On Wed, Jan 29, 2020, 08:19 Jérôme Hénin notifications@github.com wrote:

I have another use case where a partial restart is necessary, with some objects from the previous run being absent from the current one. It would be very cumbersome to carry over the information neede to redefine the objects we don't want anymore, just to delete them immediately after loading their state. It would make the generated colvars config twice as long, with useful information hidden among useless stuff.

For now I have scripts that run sed on the restart files to remove the unneeded blocks. That is of course not very satisfactory.

My current view of the ideal restarting process would be:

  • every existing object fetches its state from the state file in an order-independent way;
  • any object that does not find a saved state keeps its fresh state;
  • any data in the state file that has no corresponding object is ignored.

Any comments on whether this is a) desirable, and b) feasible?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Colvars/colvars/issues/187?email_source=notifications&email_token=ABCNSWNIFVU2DZAASGLKBWTRAF667A5CNFSM4F76GESKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKHE36Q#issuecomment-579751418, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABCNSWPLHBM4C4XX37Y5FFTRAF667ANCNFSM4F76GESA .

jhenin commented 4 years ago

What if the parsing was done sequentially and driven by whatever occurs in the state file? In naive pseudocode:

giacomofiorin commented 4 years ago

That's another way to do the same, with local offsets instead of global ones. But you still need to know ahead of time how long is the block to write its length into the header.

On Wed, Jan 29, 2020, 09:34 Jérôme Hénin notifications@github.com wrote:

What if the parsing was done sequentially and driven by whatever occurs in the state file? In naive pseudocode:

  • parse block header
  • look for corresponding object
  • if found, restart object data
  • if not found, seek end of block
  • iterate until the end of the state file

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Colvars/colvars/issues/187?email_source=notifications&email_token=ABCNSWKJEUHR3IXPOFRGRTLRAGHYZA5CNFSM4F76GESKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKHMTBA#issuecomment-579783044, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABCNSWNBUU34Z6Z3NTMCLSTRAGHYZANCNFSM4F76GESA .

jhenin commented 4 years ago

Instead of seek I meant, read (and discard) until the end of the block. That would not take any longer than parsing the whole file as is done now.

giacomofiorin commented 4 years ago

Ok that's better. We'd have to create a base-class parser common to all compute objects (colvars and biases alike) but it doesn't need to go beyond parsing the name.

On Wed, Jan 29, 2020, 09:49 Jérôme Hénin notifications@github.com wrote:

Instead of seek I meant, read (and discard) until the end of the block. That would not take any longer than parsing the whole file as is done now.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Colvars/colvars/issues/187?email_source=notifications&email_token=ABCNSWOTLY2KMPKZVEIBFATRAGJOZA5CNFSM4F76GESKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKHOGDA#issuecomment-579789580, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABCNSWOK2ETBJKOKL5MA4VDRAGJOZANCNFSM4F76GESA .

jhenin commented 4 years ago

@fhh2626 Can we consider that #320 with cv bias <name> load <filename> closes this issue ?

fhh2626 commented 4 years ago

@jhenin Yes, Thanks.

fhh2626 commented 11 months ago

Hi @jhenin , Sometimes, cv bias <name> load <filename> only works with files saved by cv bias <name> save <filename>. Sometimes this option does not work at all. Could you please check if it works? I can provide my files if you want. Haohao

giacomofiorin commented 11 months ago

Hi @fhh2626 can you specify what bias type are you experiencing trouble with? I assume it may be ABF, but better to confirm.

jhenin commented 11 months ago

Hi @fhh2626 , an example that doesn't work would be very helpful. You can send it separately if you prefer.

fhh2626 commented 11 months ago

@jhenin @giacomofiorin test_cv_load2.zip Here are my files. Neither "abf1" nor "metadynamics1" work.

Thanks. Haohao

giacomofiorin commented 11 months ago

Hi @fhh2626 cv bias <name> load is supposed to work on a file that contains data for that bias only, not for the whole module or other objects. https://colvars.github.io/master/colvars-refman-namd.html#sec:cv_command_bias_loadsave image The example above already says the first part. It just doesn't spell out the second part, i.e. that you are not allowed to load a state file for the whole module. I can try to add it if you think that this may be confusing.

fhh2626 commented 11 months ago

@giacomofiorin Oops! I have not used this feature for some time and completely forgot this! Thanks for your help!

Haohao

fhh2626 commented 7 months ago

Hi, @jhenin @giacomofiorin , I really enjoy the feature of the partial restart. However, in eABF or other extended-system based-algorithms, it is obviously that the information of the extended CVs (i.e. extended_x, extended_v) are also important when using partial restart. Is it possible to have a similar feature for extended CVs (probably something like cv bias name save <prefix>)? Thanks!

Haohao

jhenin commented 7 months ago

Hi @fhh2626 , I'm not sure I understand the need here. If you are restarting a biased simulation with a different configuration, and the cv values change drastically, the extended variable should also be reset, otherwise you could get arbitrarily high restraint energies for the extended cv, proportional to (extended_x - x)^2. Why do you want to keep these values?

fhh2626 commented 7 months ago

Hi @jhenin, For example, if I want to do replica exchange, I need to swap the structures and the extended_x values at the same time. If the extended_x values are simply reset, there will be slight bias at each swap.

在 2024年3月10日星期日,Jérôme Hénin @.***> 写道:

Hi @fhh2626 , I'm not sure I understand the need here. If you are restarting a biased simulation with a different configuration, and the cv values change drastically, the extended variable should also be reset, otherwise you could get arbitrarily high restraint energies for the extended cv, proportional to (extended_x - x)^2. Why do you want to keep these values?

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.< https://ci3.googleusercontent.com/meips/ADKq_NYiWgAv0zVkeu2dyGIx_cK7oBWY-ium6JygijrG4ItpWOO1OswK2qHsPNGSC0NofvDXUy6olGQBCN_Sz8vLd__H16JhvYkad9O5BPxXz_wxehQSEE2hoVCNEbrpon86usfMr88ticKnRAzEPu34cD0YzxDMt05oakysbA3TAErp6Zk31KKq2cZO2FlGO-28wU3Pw9mfHUWxm4GR2YMCBemFyI7i3xuvQWOnl3reYNqKbpw=s0-d-e1-ft#https://github.com/notifications/beacon/AF7FX4UDQRXB5OIAA3HN733YXQNMXA5CNFSM4F76GESKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOOZYXRHY.gif>Message ID: @.***>

jhenin commented 7 months ago

I see. Now you want to load the cv's configuration from the other replica. This could be done, although it would add a few file I/O functions and two scripting calls, just to set two variables, the extended-system value and velocity. But it would have the merit of completeness. My preferred approach would be to leave coordinates where they are and swap the biases. That should be fully enabled by the current code.

fhh2626 commented 7 months ago

Good idea! I will have a try.