-
Notifications
You must be signed in to change notification settings - Fork 1
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
Clean-up file structure and setup app to deploy Heroku #39
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.
lgtm
"id": "irish-tuning", | ||
"id": "clinical-louis", |
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.
The changes to these id's looks like some automated naming structure?
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.
this is just typical ipynb dif stuff
from app_wrangling import call_boardgame_data, subset_data | ||
|
||
# load board game data | ||
boardgame_data = call_boardgame_data() |
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.
Is this so that we are only calling the .csv once?
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.
yup, there will still need to be more work on the data logic workflow, quick update was just to ensure the csv was only being read in once
color="primary", | ||
), | ||
dbc.Collapse( | ||
dbc.Card(dbc.CardBody(data_set_descirption())), |
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.
Seems that this function has a persistent spelling mistake. Small fix
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.
added to #38
cat: list | ||
mech: list | ||
pub: list | ||
n: int | ||
|
||
return: pandas dataframe | ||
""" | ||
boardgame_data = call_boardgame_data() | ||
boardgame_data = data.copy(deep=True) |
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.
What is the advantage of having this as a .copy() versus just making boardgame_data = data?
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 just went through this quickly but anytime one is potentially doing in place modification of data, the data should be copied to avoid function side-effects of modifying the original data. This wouldn't have been a problem before as you were resetting with read.csv every time but now we're using a single loaded dataset that lives in memory. deep=True
is required as the dataframe has lists.
There may be some performance gains by not copying the dataframe where it is definitely not being modified in place. Should be done as part of the general data performance review/optimization. When I did this I just went with being safe and used copy in all instances where it looked like modifications could potentially be happening to the dataframe.
Cleaned-up file structure:
NOTE: @mqharris will need to go into the data folder and update the README and anything else required. The scripts will also likely require modification in terms of file path and the script folder should include a README providing an overview of each of the scripts - this can either be done directly within this branch as part of this PR or can be completed after this is pulled in.
Updated app so that the csv isn't read every time a function is called - the performance has improved a bit but still requires more work. Probably requires someone to sit down and look at the entire data workflow of the functions and app.
Added heroku deployment files, app is deployed here: https://boardgame-dashboard-data551.herokuapp.com/
It functions well with the exception of the top 2 figures on the first tab which will need to be optimized so the app is more responsive.
Also, definitely take a look at the reflection document and make sure everything is correct in it.