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

ltemplify.py usability improvements #17

Closed
sgsaenger opened this issue Aug 29, 2019 · 5 comments
Closed

ltemplify.py usability improvements #17

sgsaenger opened this issue Aug 29, 2019 · 5 comments

Comments

@sgsaenger
Copy link
Contributor

We've discussed this already in sgsaenger/vipster#35,
but here are some ideas i have about the architecture of your scripts.
I've mostly looked at ltemplify.py, so this is centered around this file.

It seems like the different workloads of ltemplify.main() are separated into logical blocks already.
These could easily be encapsulated in functions to increase interoperability/readability, e.g. like

def main():
    parse_pass1()
    parse_pass2()
    eval_atoms()
    eval_masses()

This usually decreases statefulness and makes it easier to export an API.

More importantly, i'd recommend using something like argparse to handle cmdline-arguments.
I've stumbled over this because ltemplify.py itself does not tell you about the (semi-mandatory?) use of the -atom_style argument.
argparse would add a -h option for free, which greatly improves discoverability.

Semi-related to this (just don't want to spam issues), would it be possible to parse the atom_style comment from the Atoms section? At least, comments like in moltemplate.sh about the guessed/default atom_style should be printed.

@jewettaij
Copy link
Owner

Ugh. I never imagined anybody would read that code. I'm wincing thinking about it. I only ever expected people would want to access it from the bash terminal. The "ltemplify.py" file had one enormous main() function, 4500 lines long. I wrote it in haste as just a quick and dirty way to get other people's LAMMPS simulation data into moltemplate format. I never realized how much it would be used, or how large it would grow.

Although I did not follow your exact recommendations, I took your general suggestions to clean up this code to heart. Instructions for accessing the functionality are included at the bottom of this message.

I also wrote up some documentation

It's still a appalling mess, but it's a somewhat easier to read appalling mess.

State:

There are an enormous number of variables in this code describing the (often complicated) state of the system described in the user's simulation files. Instead of using one ridiculously enormous function with a huge list of local variables (or global variables!), I a made a class. All these variables are now members of that class. Following your suggestions, I split the main() or ui() function into 7 smaller member functions of that class which do various things like parse the argument list, read the files, pass through various sections of the files, and write the final output file. I was still too lazy to think up good names for these functions, but at least I added some cursory docstrings.

To invoke ltemplify.py from within python use:

import moltemplate

# Read the command line arguments to figure out which files
# we will need to read (and also how to modify the output format).
ltemp = moltemplate.ltemplify.Ltemplify(argument_list)

# Using these settings, convert these files into a single file.
# Send the content that file to sys.stdout.
import sys
ltemp.Convert(sys.stdout)
# (To send to a string, I think you can use io.StringIO instead of sys.stdout.)

(I tested this recently, but please let me know if any of this fails to work.)

As for the argument list,
see documentation here

Perhaps one day, I will use sphinx and readthedocs. But currently, most of my documentation is still written in latex, and some of it eventually gets copied to the various .md files scattered throughout the "doc/" subdirectory.

Similarly, I will use argparse in future projects, but I was too lazy to rewrite the current arg-parser which is several hundred lines of code, and afraid of introducing more bugs if I did.

If you have no more suggestions, I will close this issue.

Thank you for your suggestions, and my condolences that you had to open this file.

@jewettaij
Copy link
Owner

Regarding this issue:

Semi-related to this (just don't want to spam issues), would it be possible to parse the atom_style comment from the Atoms section? At least, comments like in moltemplate.sh about the guessed/default atom_style should be printed.

Yes. This is implemented finally. Great suggestion.

@jewettaij
Copy link
Owner

May I close this issue?
(Thanks for the PR, by the way.)

@sgsaenger
Copy link
Contributor Author

Yes, please go ahead!

@jewettaij
Copy link
Owner

glad to finally get this implemented. thanks again.

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

2 participants