search

Dlux804 / McQuade-Chem-ML

Development of easy to use and reproducible ML scripts for chemistry.
5 stars 1 forks source link

Best practices #12

Closed Dlux804 closed 4 years ago

Dlux804 commented 4 years ago

Object Oriented, well commented, and functional. There are still plenty of things to add, but since this code can run and produce good results, I think it's time to merge.

Please look over the code, feed it garbage, etc. It is not unbreakable by any means.

If it does not have any major glaring issues, accept the merge. Take note of minor issues and TODOs and create GitHub issues for them.

Dlux804 commented 4 years ago

@andreshyer I feel that many of your comments were addressed in the code.

  1. Descriptastorus is a tricky install. If you look at the README it tells the user how to handle it. The requirements.txt is not the way to set up a conda env and should be removed. Instead use the mlapp.yml file.
  2. The script allows for multiple ML algorithms, all the ones that Quang's script uses.
  3. There are a lot of comments, but they are not everywhere. Please mark regions that need further explanation with # TODO and make an github issue for them.
  4. The graphing function is in its own method but not class. If you feel like it should be its own class, make an issue for it.
  5. The main part of the code is set up as a MlModel class with many of the functions calling aspects of that class. How the different functions relate to the class is not uniform and can be improved.

I think we should sit down and discuss the code base to get on the same page.