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

Add comments, suggestions and clean up to matchMinima.py, look for #M #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

magee256
Copy link

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.

@@ -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.
Copy link
Owner

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?

Copy link
Author

@magee256 magee256 Jul 12, 2017

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):
Copy link
Owner

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?
Copy link
Owner

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

Copy link
Author

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?
Copy link
Owner

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?

Copy link
Author

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

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 ... ])
Copy link
Owner

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

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

Copy link
Owner

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

@vtlim
Copy link
Owner

vtlim commented Jul 12, 2017

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.

@magee256
Copy link
Author

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.

vtlim pushed a commit that referenced this pull request Nov 29, 2017
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