-
Notifications
You must be signed in to change notification settings - Fork 376
Refactor Chemistry Drivers to also accept Molecule class #1297
Conversation
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 think that we need some more work on the Driver
interface-structure and how the Molecule
object is integrated into the drivers.
53bd810
to
1266023
Compare
b4e3d9e
to
4e51440
Compare
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 noticed a minor inconsistency on the use of unchecked strings vs Enums.
Please comment.
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.
Except for the last open discussion point the only changes I would still like to see are the docstrings in the Molecule
class to be updated. As they stand now, they only document the arguments and return types but none of them document the purpose of the methods.
The rest looks good to me.
You are referring to methods like absolute stretching etc. as the class states some overall intent and the rest are mostly simple setter/getters. The names on them seemed pretty self explanatory and the params state how the stretch or bend is accomplished via the params - so they seem as if they are mostly usable as-is - what did you have in mind to improve. This is not code I wrote so I would have to study the methods etc to see if better can be done - if someone there has more experience with this code maybe they can do this more efficiently. In the absence of that I think it will have to stand as-is. |
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.
Okay in that case this looks good to me now.
We can always go back and add more documentation as we deem necessary.
Summary
Adds the new class Molecule as an additional input to Driver classes and update those drivers so that they can build their configuration strings using the Molecule info.
Details and comments