-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/unite project with team culture #21
Feature/unite project with team culture #21
Conversation
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
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.
- pull
- code review
- test run - 80 passed, 2 skipped, 500 warnings
From the overall review, I have noted these points (as ideas only):
- Data and data_validation could be placed in the tests folder where they are used. Then, no repo structure explanation is needed in README.
- Why mention workflows? The README is for users. The developer will face it in the first PR.
- Did you think about a command line argument parser to avoid enabling inner features by editing main.py file?
- In README in How to run, I would expect sub-chapters describing how to reach variant: two variant of similarity and column2vec
- DataShow looks like three features. I saw only one line in README.
- I would add some duration value. Maybe an example of the output: 80 passed, 2 skipped, 500 warnings in 1887.83s (0:31:27)
- Consider a more detailed table of contents for users looking for exact parts.
|
d3ab9db
to
d2acca5
Compare
|
||
st.title("Uber pickups in NYC") | ||
DATE_COLUMN = "date/time" | ||
DATA_URL = "https://s3-us-west-2.amazonaws.com/" "streamlit-demo-data/uber-raw-data-sep14.csv.gz" |
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.
In the end, this gets concatenated, why have it separate?
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.
Fixed
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 saw
- 2 skipped tests
- 500 warnings
Are you planning to zero this indicators.
I will resolve warnings in future |
Closes #18