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

Tutorial Updates #46

Open
mikemhenry opened this issue Feb 25, 2021 · 5 comments
Open

Tutorial Updates #46

mikemhenry opened this issue Feb 25, 2021 · 5 comments

Comments

@mikemhenry
Copy link

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
Copy link
Member

# 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
Copy link

raimis commented Feb 26, 2021

@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.

@mikemhenry
Copy link
Author

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
Copy link
Author

@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 #47

@peastman
Copy link
Member

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

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

No branches or pull requests

3 participants