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

formatting updates to alchem tutorial #47

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mikemhenry
Copy link

I chose to just modify the script in alchemical-free-energy/files since we may switch to using Jupyter notebooks
#25
Once we settle on the changes I've proposed, I'll make the changes to alchemical-free-energy/index.md.
I also decided it would be easier to start on the script so I can run the file to ensure I don't introduce any syntax errors.
There is an issue with this tutorial but it is due to pymbar choderalab/pymbar#419

Traceback (most recent call last):
  File "/home/mmh/Projects/openmm-org/tutorials/alchemical-free-energy/files/alchemical-example.py", line 84, in <module>
    [DeltaF_ij, dDeltaF_ij, Theta_ij] = mbar.getFreeEnergyDifferences()
  File "/home/mmh/miniconda3/envs/gpu-runner-test/lib/python3.9/site-packages/pymbar/mbar.py", line 541, in getFreeEnergyDifferences
    Theta_ij = self._computeAsymptoticCovarianceMatrix(
  File "/home/mmh/miniconda3/envs/gpu-runner-test/lib/python3.9/site-packages/pymbar/mbar.py", line 1679, in _computeAsymptoticCovarianceMatrix
    check_w_normalized(W, N_k)
  File "/home/mmh/miniconda3/envs/gpu-runner-test/lib/python3.9/site-packages/pymbar/utils.py", line 358, in check_w_normalized
    raise ParameterError(
pymbar.utils.ParameterError: Warning: Should have \sum_n W_nk = 1.  Actual column sum for state 0 was 2.008930. 10 other columns have similar problems

But maybe we don't have enough overlap between states?
choderalab/pymbar#358

I assumed that there is an issue with pymbar since I'm sure this tutorial worked when it was released so we should have enough coverage between states.

@mikemhenry mikemhenry mentioned this pull request Feb 26, 2021
@peastman
Copy link
Member

Is this based on some style guideline? It isn't clear to me what the reason is for some of the changes. For example, you replaced all single quotes with double quotes. Why?

A good guideline is that when a line contains operators of different priorities, there should not be spaces around the highest priority operator(s) so the visual grouping better matches the logical grouping. For example,

pressure = 80*unit.atmospheres

is more readable than

pressure = 80 * unit.atmospheres

@mikemhenry
Copy link
Author

mikemhenry commented Mar 1, 2021

I didn't find a specific openmm style guide so I used PEP8 which recommends whitespace around the operator when there is only one. I agree for the case of i = 1*2 + 3 you want to use whitespace to help indicate operator priority, but with only one operator i = 1 * 2 is better than i = 1*2 (IMHO).

For quotes I prefer using double so I can naturally use an apostrophe in a string without having to escape it. PEP8 just says to be consistent so I like to start with " even if I don't have any ' in strings since as soon as I need to use a ' in a string, now I need to reformat my strings, or have one string that uses ".

@jchodera
Copy link
Member

jchodera commented Mar 1, 2021

+1 for PEP8!

@peastman
Copy link
Member

peastman commented Mar 1, 2021

Here is what PEP8 says about it:

If operators with different priorities are used, consider adding whitespace around the operators with the lowest priority(ies). Use your own judgment; however, never use more than one space, and always have the same amount of whitespace on both sides of a binary operator:

I agree with that, which doesn't contradict what I said: when operators have different priorities, do not use whitespace around the operators of highest priority. This improves readability.

For quotes I prefer using double so I can naturally use an apostrophe in a string without having to escape it.

In typical Python code, single quotes tend to be a lot more common than double quotes. That's for the simple reason that they're faster to type! And there isn't a string containing an apostrophe anywhere in this tutorial, or probably in any of the others either, so that isn't a concern.

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.

3 participants