Dlux804 / McQuade-Chem-ML

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

Cleaning up Main.py #73

Closed dickeygh closed 4 years ago

dickeygh commented 4 years ago

Main.py:

  1. Added importing of Get_Classification.py (line 3).
  2. Changed classification workflow to include less if statements (these if statements are now largely performed in Get_classification.py instead of in main.py).
  3. Re-formatted main.py to be neater where possible.

Get_Classification.py:

  1. Added this new file in order to help clean up main.py by putting if statements crucial to classification in a different file.
pep8speaks commented 4 years ago

Hello @dickeygh! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 3:80: E501 line too long (101 > 79 characters) Line 4:80: E501 line too long (104 > 79 characters) Line 6:80: E501 line too long (113 > 79 characters) Line 7:80: E501 line too long (89 > 79 characters) Line 8:80: E501 line too long (97 > 79 characters) Line 9:80: E501 line too long (115 > 79 characters) Line 10:80: E501 line too long (106 > 79 characters) Line 11:80: E501 line too long (100 > 79 characters) Line 13:80: E501 line too long (99 > 79 characters) Line 15:80: E501 line too long (96 > 79 characters) Line 27:1: E303 too many blank lines (3) Line 29:80: E501 line too long (81 > 79 characters) Line 32:80: E501 line too long (81 > 79 characters) Line 35:80: E501 line too long (81 > 79 characters) Line 38:17: W292 no newline at end of file

Line 31:9: E303 too many blank lines (2) Line 65:69: E261 at least two spaces before inline comment Line 65:80: E501 line too long (141 > 79 characters) Line 68:46: E261 at least two spaces before inline comment Line 72:80: E501 line too long (111 > 79 characters) Line 78:80: E501 line too long (81 > 79 characters) Line 79:80: E501 line too long (109 > 79 characters) Line 100:80: E501 line too long (149 > 79 characters) Line 102:80: E501 line too long (87 > 79 characters) Line 123:80: E501 line too long (88 > 79 characters)

Comment last updated at 2020-07-21 18:16:14 UTC
andreshyer commented 4 years ago

Hey Gabe, I looked at this PR, and the there are no bugs in the code or anything like that. But, something that I think would really help out main.py is to not have classification and regression so split. Right now, we have to specify weather to run regression or classification. I think the best way to clean up main.py is to run from like a master list of datasets (regradless of regression or classification). That way we can just hit run, and it will run through all the regression and classification models at once. Then if we only want to run a few regression model, we tweak the master list, and vis-versa for classification.

Main.py definitely needs some love, and this is a good start. I think it is not the best pratice having such a hard split between regression and clasification, unless there is a reason for such a hard split.