Input file support for `target_acceptance` for Monte-Carlo moves #94

g-bauer · 2017-01-24T13:44:00Z

This PR adds input file support for adding MC moves with a target acceptance (see #23). Also, the update_amplitude_frequency - which is needed to achieve the acceptance - can be set from the input.

Extract unsigned integer from TOML
Add key for target_acceptance
Add key for ~~update_delta_every (since all our displacements are called delta)~~ update_frequency
Add example input file
Add tests in input/tests
Add documentation

Open questions:

How to handle the case, where an acceptance is given, but no update_frequency? Set default? Error?
- Solution: Throw error telling the user to specify update_frequency.
How to handle the case, where an update_frequency is given, but no acceptance? Ignore? Set default for acceptance?
- Solution: Accept input. Updates don't change scaling factor.

I'd be happy to discuss naming of keys and any other kind of feedback.

Luthaf · 2017-01-26T20:53:54Z

src/input/src/simulations/mc.rs

+        if config.get("update_delta_every").is_some() {
+            let update_amplitude = try!(extract::uint(
+                    "update_delta_every", config, "Monte-Carlo propagator"));
+            mc.set_amplitude_update_frequency(update_amplitude);


I would use the same name in input and code. Either update_delta_every or update_frequency (I prefer the second), but the same one.

This help users of the code to dive in and write rust code: they are already familiar with the naming of functions.

Luthaf · 2017-01-26T20:55:04Z

doc/src/input/mc.md

+temperature with unit.
+
+- Optional keys:
+    * `update_delta_every` (uint): After this number of steps of a move, `delta` values 


unsigned integer or positive integer or just integer might be more readable than uint here.

Luthaf · 2017-01-26T20:57:02Z

examples/ethane.toml

@@ -0,0 +1,41 @@
+# run via:
+# cargo run ethane.toml --release


Should we merge this with the mc_npt_ethane.rs example ?

What do you mean by "merge"? Use this instead of the other example?

I believe both examples run the same simulation? If so the .rs file could just read this input and run the corresponding simulation.

Yepp, they both run the same simulation. I thought it would be good to show how to do the exact same thing using both methods.

We can change the .rs though if that makes more sense.

OK, let's keep it. I am a bit worried about examples proliferation, I don't know what is the best way to showcase the code. Too many examples might be overwhelming, and too few is worst.

But we can figure it later!

That's true. I think having a lot of examples is a good thing for now. We can sort out examples showing features later.

Luthaf · 2017-01-26T21:02:04Z

How to handle the case, where an acceptance is given, but no update_frequency? Set default? Error?
How to handle the case, where an update_frequency is given, but no acceptance? Ignore? Set default for acceptance?

I would go for the conservative way: accept update_frequency without acceptance, but not the reverse.

Luthaf · 2017-02-09T12:40:02Z

doc/src/input/mc.md

-    {type = "Translate", delta = "1 A", frequency = 2},
-    {type = "Rotate", delta = "20 deg", molecule = "CO2.xyz"},
-    {type = "Resize", pressure = "10 bar", delta = "3 A^3", frequency = 0.001},
+    {type = "Translate", delta = "1 A", frequency = 2, target_acceptance = 0.5},


I prefer if we do not have target_acceptance in the main examples, but only in the corresponding documentation. Else people will think it is mandatory and start to copy-paste it everywhere.

Sure, I can change that. From my experience, setting an acceptance (or having it set to 0.5 by the program) is the default behavior in most codes I know.

Luthaf · 2017-02-09T12:42:19Z

src/input/tests/simulation/bad/mc/move_acceptance-3.toml

+update_frequency = -1
+#^ 'update_frequency' must be a positive integer in Monte-Carlo propagator
+moves = [
+    {type = "Rotate", delta = "6 A", frequency = 0.3, target_acceptance = 0.5}


Could you add "bad" tests setting target_acceptance to something below 0 and above 1?

Sure. Currently, this is not checked within input but in core. It will panic with a hint to the mcmove source file. Should we catch this in the input directly?

Yes, panics in core are more to check that the API is respected, but we should always respect the API from the input crate. I think it is better for the user experience to get errors instead of panics.

Luthaf · 2017-02-09T12:44:34Z

@g-bauer, could you also squash together related commits (the fix whitespace ones; the add examples ones; ...)

g-bauer · 2017-02-09T20:04:02Z

@g-bauer, could you also squash together related commits (the fix whitespace ones; the add examples ones; ...)

I'll try. Fingers crossed that I'm not killing my branch. My commits are still a mess, i vow improvement 😄

Edit: Aaaaaand I forgot to add the new input tests 😞

Added example.

Fixed whitespace in example.

Updated documentation due to review. Added more input tests. Fixed newlines.

Luthaf · 2017-02-09T20:44:21Z

Edit: Aaaaaand I forgot to add the new input tests 😞

Did you? I see them in the last commit. Or you pushed a change after your comment?

g-bauer · 2017-02-09T20:45:23Z

Or you pushed a change after your comment?

🙄

Luthaf reviewed Jan 26, 2017

View reviewed changes

g-bauer added 2 commits February 8, 2017 21:27

Add extraction of uint from TOML.

eac3b06

Add input support for move acceptance and update frequency.

f25a931

g-bauer force-pushed the input_mcmove_acceptance branch from 293895e to 1c28a13 Compare February 8, 2017 21:26

g-bauer changed the title ~~[WIP] Input file support for target_acceptance for Monte-Carlo moves~~ Input file support for target_acceptance for Monte-Carlo moves Feb 8, 2017

Luthaf requested changes Feb 9, 2017

View reviewed changes

g-bauer mentioned this pull request Feb 9, 2017

Benchmarking Lumol #62

Open

22 tasks

g-bauer added 7 commits February 9, 2017 21:06

Added example input file.

87c630b

Added example.

Added documentation.

1e4ff62

Changed key to update frequency.

158c9f9

Better error message for uint.

b10c4d2

Catch missing update_frequency when target_acceptance is specified.

36054d0

Added tests.

9be73d6

Fixed whitespaces and newlines.

d207433

Fixed whitespace in example.

g-bauer force-pushed the input_mcmove_acceptance branch 2 times, most recently from 91f2aaa to 14f91fa Compare February 9, 2017 20:14

Changes due to review.

f7821ed

Updated documentation due to review. Added more input tests. Fixed newlines.

g-bauer force-pushed the input_mcmove_acceptance branch from 14f91fa to f7821ed Compare February 9, 2017 20:21

Luthaf approved these changes Feb 9, 2017

View reviewed changes

Luthaf merged commit 05f12f4 into lumol-org:master Feb 9, 2017

g-bauer deleted the input_mcmove_acceptance branch February 9, 2017 20:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Input file support for `target_acceptance` for Monte-Carlo moves #94

Input file support for `target_acceptance` for Monte-Carlo moves #94

g-bauer commented Jan 24, 2017 •

edited

Loading

Luthaf Jan 26, 2017

Luthaf Jan 26, 2017

Luthaf Jan 26, 2017

g-bauer Jan 27, 2017

Luthaf Jan 28, 2017

g-bauer Jan 28, 2017

Luthaf Jan 29, 2017 •

edited

Loading

g-bauer Jan 29, 2017

Luthaf commented Jan 26, 2017

Luthaf Feb 9, 2017

g-bauer Feb 9, 2017

Luthaf Feb 9, 2017

g-bauer Feb 9, 2017

Luthaf Feb 9, 2017

Luthaf commented Feb 9, 2017

g-bauer commented Feb 9, 2017 •

edited

Loading

Luthaf commented Feb 9, 2017

g-bauer commented Feb 9, 2017

Input file support for target_acceptance for Monte-Carlo moves #94

Input file support for target_acceptance for Monte-Carlo moves #94

Conversation

g-bauer commented Jan 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luthaf Jan 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luthaf commented Jan 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luthaf commented Feb 9, 2017

g-bauer commented Feb 9, 2017 • edited Loading

Luthaf commented Feb 9, 2017

g-bauer commented Feb 9, 2017

Input file support for `target_acceptance` for Monte-Carlo moves #94

Input file support for `target_acceptance` for Monte-Carlo moves #94

g-bauer commented Jan 24, 2017 •

edited

Loading

Luthaf Jan 29, 2017 •

edited

Loading

g-bauer commented Feb 9, 2017 •

edited

Loading