openmm / openmm-org

Content of https://openmm.org
Other
4 stars 11 forks source link

Tutorial Updates #46

Open mikemhenry opened 3 years ago

mikemhenry commented 3 years ago

Wanted to make an issue before making a PR updating the tutorials to more closely follow PEP8 and use modern features of the python language. Since python3.6 is the oldest version of python supported in the next openmm release (at least that is my understanding) I would not use any features newer than those available in 3.6.

My motivation is that most users will copy and paste a tutorial and adapt things to fit their workflow, so we should try and incorporate python programming best practices.

Two quick examples from the alchemical free energy tutorial.

# Defining alchemically-modified potentials
sigma = 3.4*unit.angstrom; epsilon = 0.238 * unit.kilocalories_per_mole
# Use a comma to define related things close together 
sigma, epsilon = 3.4*unit.angstrom,  0.238 * unit.kilocalories_per_mole

# Simulating alchemically-modified systems
print('state %5d iteration %5d / %5d' % (k, iteration, niterations))
# use f-string formatting, print the current state number, and take care of starting at 0 and not 1
print(f"state {k} / {nstates-1} iteration {iteration} / {niterations-1}")

I'm happy to submit a PR for each tutorial and we can discuss each change on a case-by-case basis.

peastman commented 3 years ago

# Use a comma to define related things close together

I find the former clearer, since it doesn't separate the name sigma from the value you're assigning to it. In fact, I'd split this into two lines:

sigma = 3.4*unit.angstrom
epsilon = 0.238*unit.kilocalories_per_mole

# use f-string formatting, print the current state number, and take care of starting at 0 and not 1 print(f"state {k} / {nstates-1} iteration {iteration} / {niterations-1}")

Agreed about the formatting, not about the -1. If you print "state 0 / 9", that indicates there are 9 states when in fact there are 10.

raimis commented 3 years ago

@mikemhenry thanks for your interest! The tutorials would definitely benefit from some updates and clean up.

I think that we don't need so much comment about the language features. If they want to learn Python, there are better tutorial for that.

# Use a comma to define related things close together 
# use f-string formatting, print the current state number, and take care of starting at 0 and not 1

Also, keep in mind, there are more tutorials (https://github.com/openmm/openmm-org/issues/25). At some point, we may convert these one to Jupyter notebooks too.

mikemhenry commented 3 years ago

Sounds good! I'll take a pass at things and I really appreciate the feedback!

There are 10 states when going 0 to 9, what looks weird is when the loops end like:

9/10
0/10

I think most people would expect an iteration to look like

9/9
0/9

or

10/10
1/10
mikemhenry commented 3 years ago

@mikemhenry thanks for your interest! The tutorials would definitely benefit from some updates and clean up.

I think that we don't need so much comment about the language features. If they want to learn Python, there are better tutorial for that.

# Use a comma to define related things close together 
# use f-string formatting, print the current state number, and take care of starting at 0 and not 1

Also, keep in mind, there are more tutorials (#25). At some point, we may convert these one to Jupyter notebooks too.

That is a good point, I'll focus on the scripts so that if we decide to switch to notebooks we can import the code so we don't need to copy and paste/maintain it in two locations. If we don't end up doing it by the time we are happy with the tutorials then I'll split things up and copy it back into index.md

PR opened https://github.com/openmm/openmm-org/pull/47

peastman commented 3 years ago

Adding 1 to k instead of subtracting 1 from nstates seems clearest. That way it counts from 1/10 up to 10/10.