Skip to content
This repository was archived by the owner on Oct 28, 2022. It is now read-only.

Task2#6

Open
ZuodongWang wants to merge 15 commits into
masterfrom
task2
Open

Task2#6
ZuodongWang wants to merge 15 commits into
masterfrom
task2

Conversation

@ZuodongWang

Copy link
Copy Markdown
Collaborator

No description provided.

@pep8speaks

pep8speaks commented Oct 25, 2019

Copy link
Copy Markdown

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

Line 23:1: E303 too many blank lines (3)
Line 25:35: E203 whitespace before ':'
Line 26:35: E203 whitespace before ':'
Line 27:1: W293 blank line contains whitespace
Line 32:13: E128 continuation line under-indented for visual indent
Line 33:13: E128 continuation line under-indented for visual indent
Line 34:13: E128 continuation line under-indented for visual indent
Line 35:13: E128 continuation line under-indented for visual indent
Line 35:30: W291 trailing whitespace
Line 36:13: E128 continuation line under-indented for visual indent
Line 37:9: E124 closing bracket does not match visual indentation
Line 41:1: W293 blank line contains whitespace
Line 42:80: E501 line too long (94 > 79 characters)
Line 47:5: E303 too many blank lines (2)
Line 49:1: E302 expected 2 blank lines, found 1
Line 60:10: E225 missing whitespace around operator
Line 67:10: E225 missing whitespace around operator
Line 68:1: W293 blank line contains whitespace
Line 75:1: E302 expected 2 blank lines, found 1
Line 85:10: E225 missing whitespace around operator
Line 92:10: E225 missing whitespace around operator
Line 93:1: W293 blank line contains whitespace
Line 99:1: W293 blank line contains whitespace
Line 100:1: E302 expected 2 blank lines, found 1
Line 110:1: W293 blank line contains whitespace
Line 117:1: W293 blank line contains whitespace
Line 126:5: E303 too many blank lines (2)
Line 126:80: E501 line too long (85 > 79 characters)
Line 127:80: E501 line too long (84 > 79 characters)
Line 127:85: W291 trailing whitespace
Line 148:1: W293 blank line contains whitespace
Line 149:80: E501 line too long (93 > 79 characters)
Line 159:80: E501 line too long (87 > 79 characters)
Line 165:5: E303 too many blank lines (2)
Line 170:80: E501 line too long (89 > 79 characters)
Line 181:80: E501 line too long (85 > 79 characters)
Line 182:1: W293 blank line contains whitespace
Line 184:5: E303 too many blank lines (2)
Line 189:1: E303 too many blank lines (4)
Line 190:1: W293 blank line contains whitespace
Line 194:5: E303 too many blank lines (2)
Line 195:80: E501 line too long (94 > 79 characters)
Line 202:9: E303 too many blank lines (2)
Line 206:9: E303 too many blank lines (2)
Line 219:5: E265 block comment should start with '# '
Line 220:5: E731 do not assign a lambda expression, use a def
Line 224:80: E501 line too long (120 > 79 characters)
Line 224:96: E261 at least two spaces before inline comment
Line 224:96: E262 inline comment should start with '# '
Line 228:1: E303 too many blank lines (3)
Line 231:80: E501 line too long (142 > 79 characters)
Line 231:119: E261 at least two spaces before inline comment
Line 231:119: E262 inline comment should start with '# '
Line 236:1: W293 blank line contains whitespace
Line 245:1: E303 too many blank lines (3)
Line 248:34: E231 missing whitespace after ','
Line 249:1: W391 blank line at end of file

Comment last updated at 2020-02-25 03:25:14 UTC

task 2b draft
@Will-Shanks

Copy link
Copy Markdown
Owner

resolves #2 (linking this PR w/ issue)

reda2367 and others added 3 commits November 12, 2019 18:37
This code could be cleaned up A LOT, but all the functionality is there. It ignores the 1 bottom and 1 top percent of word heights

@JadeWibbels JadeWibbels left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi guys! As promised I am going to be adding code review requests. The files in task2 seem to have a LOT of overlap on functionality. Making things more modular would mean that there is less duplication of code/effort. We also need unit tests!
I would be happy to meet with you in small groups or one on one if you have questions on any comments. Also! If you think that things are perfect the way they are feel free to reply to my comments and explain the whys!

Comment thread tasks/task2/draw2.py Outdated

logging.basicConfig(level=logging.INFO)

def getColumnDelimiters(fName):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is some good documentation here, but could use more parameter information. There are also some opportunities for splitting out smaller functions to increase modularity.

Comment thread tasks/task2/draw2.py Outdated
return columnSeperatorXVals


def main():

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar comments on encouraging modularity.

Comment thread tasks/task2/draw2.py Outdated


if __name__ == "__main__":
main() No newline at end of file

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unit tests please!

Comment thread tasks/task2/draw2_styled.py Outdated
@@ -0,0 +1,133 @@
#!/usr/bin/env python3

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file looks like it contains a lot of the same functionality as the previous file. If you find you are re writing and re using the same code in multiple areas, I would recommend splitting it out and making it more modular.

Comment thread tasks/task2/draw2_styled.py Outdated


if __name__ == "__main__":
main()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unit tests please!

@Will-Shanks Will-Shanks linked an issue Feb 11, 2020 that may be closed by this pull request
@kevmonsta kevmonsta linked an issue Feb 25, 2020 that may be closed by this pull request
Creates unified day files. Currently this runs insanely slow on summit. Although the current version has not been run yet and is much simpler. The 4 lines of code do run pretty fast on a local machine.

@Will-Shanks Will-Shanks left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be helpful if comments were added to the code to make it more readable.
Could you please also change the filenames to better reflect what they do. Since we have multiple job submission scripts it would be helpful to denote what this job in particular does in the submit script name, same goes for the draw5.py filename.

@kevmonsta

Copy link
Copy Markdown
Collaborator

I am not fully sure what this code does.

It looks like we need Zoudong to add some comments to the code and I would also like to request that we get a description of what this code does.

@Will-Shanks

Copy link
Copy Markdown
Owner

I am not fully sure what this code does.

It looks like we need Zoudong to add some comments to the code and I would also like to request that we get a description of what this code does.

I believe this is actually the branch Remy has been working on not Zoudong, so it probably makes more sense to have him write a description of what it does.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Create Unified Day Files task2

8 participants