Skip to content

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.



#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):


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

"""

#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.

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

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