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

[WIP]Tutorial for pair potential implementation #196

Merged
merged 6 commits into from
Oct 19, 2017

Conversation

g-bauer
Copy link
Contributor

@g-bauer g-bauer commented Sep 11, 2017

EDIT: New structure

This PR will teach users how to add a PairPotential to Lumol.

These are the steps discussed in the tutorial:
Part 1:

  • Introduction
  • Prerequisites
  • Building the struct
  • impl Potential
  • impl PairPotential
  • example simulation

Part 2 (moving the implementation into lumol core):

  • add API documentation
  • add unit tests
  • add parser for input files
  • add documentation in the manual

You can clone this PR and locally build the manual using

mdbook serve --open

from within the lumol/doc/ directory (you can install mdbook using cargo).


In this tutorial, we will implement a potential to describe non-bonded pair interactions, namely the [**Mie potential**](http://www.sklogwiki.org/SklogWiki/index.php/Mie_potential). We will have to implement both the `Potential` as well as the `PairPotential` traits. If we wanted to implement a function that can be used as non-bonded as well as bond-length potential, we'd have to implement `Potential`, `PairPotential` as well as `BondPotential`. "Implementing a trait" means that we will define a [structure](https://doc.rust-lang.org/book/second-edition/ch05-00-structs.html) (a `struct`) for which we will add functions to satisfy the traits' requirements.

Let's start by having a look at the API documentation for `Potential`. Open the [API documentation](http://lumol.org/lumol/latest/lumol/index.html) and navigate to the `energy` module which is listed under the `modules` section at the bottom of the page. Traits are listed at the very bottom of the module documentation. Open the documentation for the `Potential` trait.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll give a direct link to the trait here, instead of making the people navigate

- $\epsilon$ the energetic paramater,
- $\sigma$ the particle diameter or structural parameter.

We start by defining the `struct` for our potential. We will add our code to the `functions.rs` file located in `lumol/src/core/src/energy`. Add the following lines:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to add new potentials in the same module as the other ones. Actually, we don't even need to add it in the same crate. See my suggestion below!


```rust
impl Mie {
fn new(sigma: f64, epsilon: f64, n: f64, m: f64) -> Mie {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the Mie::new function is enforcing some constraint, maybe the fields should not be public


fn tail_virial(&self, cutoff: f64) -> f64 {
if self.m <= 3.0 {
panic!("No tail correction possible for Mie potential with an exponent lower than 3.0")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other long ranged potentials, we return from tail_energy/tail_virial instead of panicking.

@Luthaf
Copy link
Member

Luthaf commented Sep 11, 2017

This looks very nice!

I have a suggestion about the organisation of this tutorial. Lumol is written such as it can be extended from the outside; which means the users do not need to write code in this repository to have it work with the rest of the engine. The tutorial could be written to use this property: just create a new cargo project, make it depend on the lumol crate, and implement the potential here.

This can have two benefits:

  • the tutorial can be made into a real repository that people can clone, and that can be tested with respect to lumol changes.
  • the tutorial does not need to deal with "extra work" that is needed for inclusion in this repository, but not when one just want to make the code work.

Your tutorial outline could then be splitted in two parts: the basics with

Introduction
Prerequisites
Building the struct
impl Potential
impl PairPotential

and then the extra, only needed to include code in the main repository

add API documentation
add unit tests
add parser for input files
add documentation in the manual

The only issue is until we get a plugin system (#45), users needs to write there own main function if they want to use any newly developed potential/MC move/propagator/...

@g-bauer
Copy link
Contributor Author

g-bauer commented Sep 12, 2017

Thanks for the feedback!

The tutorial could be written to use this property: just create a new cargo project, make it depend on the lumol crate, and implement the potential here.

That's a good idea, I reorganize the tutorial accordingly. Would you still have this as part of the manual (I would, see the 2nd part about docs and input) or would we treat tutorials of this kind differently? Where would you put the tutorial repo (create a tutorials dir in lumol root or have this in another repo lumol tutorials)?

I'll keep on pushing to this PR - it already includes all codes necessary to put them into a separate repo so this will be a quick change in the end.

@Luthaf
Copy link
Member

Luthaf commented Sep 12, 2017

Concerning organisation, I think we can have everything in this repo for now, so the text in the manual, and the code in a tutorials directory. If it gets annoying, we can still move it to a separated repo later.

@g-bauer
Copy link
Contributor Author

g-bauer commented Oct 12, 2017

Added first part of the tutorial. I also added a tutorials directory in the root with a separate package that implements the discussed material as well as a small sample simulation setup to test the implementation.

I somehow screwed up the git history (sorry).

Not sure how long it will take me to write the second part. Maybe it makes sense to merge this as is.

@g-bauer g-bauer changed the title [WIP]Tutorial on PairPotential impl. Intro and impl. [WIP]Tutorial on for pair potential implementation Oct 12, 2017
@g-bauer g-bauer changed the title [WIP]Tutorial on for pair potential implementation [WIP]Tutorial for pair potential implementation Oct 12, 2017
Copy link
Member

@Luthaf Luthaf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a few comment, take them or leave them as you wish!

In this tutorial we will teach you how to use your own potential function with Lumol.
This tutorial consists of two parts.
In the first part, we will implement the logic in a separate project using Lumol as an external library.
The second part describes the neccessary steps to implement the logic into Lumol `core`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

into Lumol core => into Lumol core package called lumol-core


A potential is a function that describes the energy and force between interaction sites.
In Lumol we differentiate between two types of potentials: First, there are potential functions that need the global state of the system, *i.e.* all positions, as input. The Ewald summation which is used to compute electrostatic interactions is an example for this kind of potential.
In Lumol we call them `GlobalPotential`s.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Lumol we call them ... => We call them ...

More specific, we will implement a potential to compute van-der-Waals interactions between pairs of particles.
We will make liberal use of the API documentation of both the Rust standard library as well as the Lumol API.
Please note that we will point to other references, such as the Rust book, concerning general Rust concepts to keep this tutorial brief.
If you have questions concerning Rust or Lumol, please don't hesitate to file an issue on github or join the discussion on our gitter.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe add the links to gitter and github here

@@ -0,0 +1,3 @@
/target/
**/*.rs.bk
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the standard gitignore created by cargo new?

@@ -0,0 +1,7 @@
[package]
name = "mie_potential"
version = "0.1.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use version = "0.0.0" here, and add a publish = false to ensure this is never published to crates.io

We start by creating a new package using `cargo`:

```
cargo new mie_potential
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as below, I would call the crate lumol-tutorial-potential


We start by defining the `struct` for our potential. Add the following lines to `lib.rs`:

```rust
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it would be possible to directly include the relevant part of the file in /tutorials ? This would make it easier to prevent the code from bit rotting.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm note sure what you mean here. How would you directly include that part of the file?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I checked and this is not possible with markdown. So let's keep it this way for now.

I do this with sphinx: you have example files somewhere, and then you include them in your documentation. (see here for example). This allow to ensure that the documentation and the actual files stays in sync.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see, yes i do that in sphinx too.

/// Exponent of attractive contribution
m: f64,
/// Energetic prefactor computed from the exponents and epsilon
prefac: f64,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not prefactor?

}
```

First, we can see that `PairPotential` enforces the implementation of `Potential` which is denoted by `pub trait PairPotential: Potential ...` (we ignore `BoxClonePair` for now).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can add a comment saying that BoxClonePair is automatically implemented for us if we implement PairPotential manually?

In fact, the potential has to be a *short ranged* potential.
For our Mie potential, both the exponents have to be larger than 3.0 else our potential will be *long ranged* and
the integral that has to be solved to compute the tail corrections diverges.
For now, we will simply return zero in this case.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "for now" looks strange here: returning 0 is the right thing to do if the potential is not short-ranged

@Luthaf
Copy link
Member

Luthaf commented Oct 12, 2017

I somehow screwed up the git history (sorry).

No, this does not look like so =)

Not sure how long it will take me to write the second part. Maybe it makes sense to merge this as is.

Yeah, we can merge this as it is, and keep the advanced part of the tutorial for later.


Can you also add a test in .travis.yml that check that the code compiles (you don't need to run the simulation, just build everything with cargo build), and that it keeps compiling even when we will change other things?

Renamed project and make use of lumol_input

Suggested changes and exclude tutorial from workspace

Add script to build tutorials

Added suggestions from reviewer
.travis.yml Outdated
@@ -29,8 +29,12 @@ script:
# Run unit tests and doc tests in debug mode
- cargo test -p lumol-core
- cargo test -p lumol-input
# Check if tutorials compile
- cd
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a previous attempt leftover, but this might break the build, as the code is not in $HOME.

.travis.yml Outdated
# Check that benchmarks still compile
- cargo bench --no-run
# Check if tutorials compile
- ./scripts/ci/compile-tutorials.py
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather explicitly do cd tutorial/lumol_potential_tutorial && cargo build here. We can craft a script for this later!

You might need to do a cd $TRAVIS_BUILD_DIR after that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, that was my first attempt :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is another way to do this, in one line: cargo build --manifest-path=tutorial/lumol_potential_tutorial/Cargo.toml.

But if you prefer the python script, you'll need to chmod +x it. There is a permission denied error on Travis.

Copy link
Member

@Luthaf Luthaf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. If you are done with it, I'll merge =)

@g-bauer
Copy link
Contributor Author

g-bauer commented Oct 19, 2017

I'm done with this part.

@Luthaf Luthaf merged commit 741b23d into lumol-org:master Oct 19, 2017
@Luthaf
Copy link
Member

Luthaf commented Oct 19, 2017

And thank you again for doing this: having a good documentation is essential, especially for open source software!

@g-bauer g-bauer deleted the doc_add_potential branch October 19, 2017 14:49
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 this pull request may close these issues.

2 participants