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

2DRMSD #56

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

2DRMSD #56

wants to merge 4 commits into from

Conversation

aneesmkp
Copy link

@aneesmkp aneesmkp commented Jun 8, 2021

2drmsd.py script to calculate 2D RMSD after aligning with a different selection.

2drmsd.py script to calculate 2D RMSD after aligning with a different selection.
@agrossfield
Copy link
Member

agrossfield commented Jun 8, 2021

Hi Anees,

This tool looks pretty good -- this is definitely something I want to have in loos. I have a few things that I'd like to see changed before merging, though:

  • You need to write a --fullhelp message. You can see examples in tools like scattering.py in Packages/PyLOOS to see both how to do it and what the content should be. Basically, define a function called fullhelp in the main program and a small subclass that does the work (you can literally cut and paste the one in scattering.py), then create a fullhelp argparse entry and use the class as the custom action. Alternatively, I recently put in an experimental set of pre-written command-line options -- to use that approach, take a look at the options setup in all_contacts.py.
    As for content, you'll want to cover what the code does and when you'd want to use it. You should mention that if the region you're aligning and the region you're calculating the rmsd over are the same, you should use rmsds instead, since it should be much faster. If there are any potentials gotchas (eg stuff you assume the user won't do, or stuff your program assumes the user has already done), make sure the mention them.

  • Please change all tabs to 4 spaces (do it as a global search and replace so you don't miss any).

  • Please try to make lines stay under 80 char for the most part. This is mostly an issue where you did the command line arguments. I'd suggest reformatting like this:

parser.add_argument('--align',
                                   help="Align selection 1 for 1st trajectory (default = CA)",  
                                   default= 'name == "CA"', 
                                   type = str)

NOTE: the markdown is messing up the spacing for some reason, but it's supposed to have all of the arguments lined up under the first --align.

  • Along the same lines, I suggest running the code through a linter (or adding one to your editor, depending on what you use), to clean up little formatting things like missing spaces for assignments, extra blank lines. flake8 is a simple one that is available for package managers, or you could use Google's black. However, you should know that black will undo line breaking like I suggested in the previous bullet.

  • You don't absolutely have to do this, but loos.parseRangeList will handle parsing the skip, stride, range stuff, so you don't have to do it.

  • You should probably use the pyloos.Trajectory class instead of raw trajectory. If you feed it the skip and stride values when you create it, you don't have to manually manage them later (you just loop over the frames). It'll look something like

    traj = loos.pyloos.Trajectory(args.traj_file, system,
                                  skip=args.skip, stride=args.stride)

or if you give skip, stride and range you'd have something like iterator=range(skip, stop, stride) instead of the skip and stride assignments.

  • The program should write the command line back to stdout. If you choose to use the new options framework, there's a function called header that will create the string. Otherwise you can do something like header = " ".join(sys.argv), which is almost as good (the function embeds each argument in quotes, which is a bit more robust). In any event, the point is that your output should contain a record of what you gave as input.

  • Put the "This file is part of LOOS" message at the top of the file. You can copy it from any of the other programs in Packages/PyLOOS, but make sure to update the copyright date and put your name in.

Feel free to ping me if you want to chat about any of this (either here or via email). I know this looks like a lot of stuff I'm asking for, but nearly all of the changes should be really easy,

Cheers,

Alan

@aneesmkp aneesmkp closed this Jul 17, 2022
@aneesmkp aneesmkp reopened this Jul 17, 2022
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