-
Notifications
You must be signed in to change notification settings - Fork 3
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
Add comments, suggestions and clean up to matchMinima.py, look for #M #1
base: master
Are you sure you want to change the base?
Conversation
@@ -19,6 +19,8 @@ | |||
|
|||
|
|||
|
|||
#M Do what OE does and define your own molecule objects, they could even | |||
#M extend the OE chem definitions to save you work. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you mean by this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OE seems to have a molecule object. If you wanted to you could define your own personal molecule object that inherits from the OE molecule. That would mean your new object would have all the methods and attributes an OE molecule object has, plus whatever you wanted to add. Declare it like class MyMolecule(OEMolecule):
allIndices = [] # for M mols, N reference minima of each mol, P matching indices for each ref minimia | ||
elists = [] # 2D list: K mols per file x J numFiles | ||
tlists = [] # 2D list: K mols per file x J numFiles | ||
refNumConfs = [] # number of conformers for each mol in reference file | ||
molNames = [] # name of each molecule. for plotting. | ||
|
||
for i, sdfQuery in enumerate(sdfList): | ||
qthry = thryList[i] | ||
for sdfQuery, qthry in zip(sdfList,thryList): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch
@@ -432,13 +395,15 @@ def getRatioTimes(allMolTimes, zeroes): | |||
|
|||
""" | |||
|
|||
#M Unrelated: You're compring times spent in each minima? What for? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm taking into acct how long it takes to optimize each conformer of a molecule. then getting an average per molecule per level of theory. that's what's being compared, the diff levels of theory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok so you're interested in the time it takes for optimization as well as the energy differences between levels of theory.
relByFile = [] | ||
sdByFile = [] | ||
for i, molist in enumerate(allMolTimes): | ||
molTimes = [] | ||
molStds = [] | ||
for j, filelist in enumerate(molist): | ||
rels = np.asarray(filelist)/np.asarray(molist[0]) | ||
#M I trust this is safe? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you mean by safe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does eliminating nan's like that bias your results? I assume some conformers don't finish optimization because of a timeout.
# if len(trimE) != len(zeroes): | ||
# print len(trimE), zeroes | ||
# sys.exit("Error in determining reference confs for molecules.") | ||
#M Delete unused code unless you expect to add it back in soon. Reduces clutter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a work in progress
@@ -580,14 +515,20 @@ def reorganizeSublists(theArray,allMolIndices): | |||
minimaE = [] | |||
for i, molIndices in enumerate(allMolIndices): | |||
molE = [] # all conf energies from ith mol in all files | |||
#M Try this: | |||
#M flipped = np.array(theArray[i]).T | |||
#M molE.append([ nan if (x == None or x == -2) else theArray[i][j][k] for ... ]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
intriguing
#M I take it there's no better way to do this? | ||
#M You should take a look at: https://docs.python.org/2/library/unittest.html | ||
#M I haven't used it myself but I really should. Also look into using assert | ||
#M statements |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually need to take this out, this was from an older version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the other two references
this was helpful, thanks. broader concepts like defining a class would be useful but probably not worth spending the time on for this particular script. will keep in mind for other applications. |
Yeah at this point it might take a while to rewrite the script to use classes. Classes do tend to make code shorter and more organized so it's good to think about where they could be used when starting a project. |
Here's my feedback. I gave myself like an hour and a half so it might not be too thorough but I think I suggested some pretty specific improvements. A lot of it is just style based though.