Skip to content
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

Added MN util file, updated MN notebook README, uploaded a clean version of MN EDA #36

Merged
merged 11 commits into from
Nov 27, 2023

Conversation

yuzhouw313
Copy link
Contributor

No description provided.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

notebooks/MN_dataset_analysis.ipynb Outdated Show resolved Hide resolved
notebooks/README.md Outdated Show resolved Hide resolved
notebooks/README.md Outdated Show resolved Hide resolved
utils/MN_util.py Outdated Show resolved Hide resolved
utils/MN_util.py Outdated Show resolved Hide resolved
notebooks/MN_EDA.ipynb Show resolved Hide resolved
notebooks/MN_EDA.ipynb Show resolved Hide resolved
Copy link
Contributor

@trevorspreadbury trevorspreadbury left a comment

Choose a reason for hiding this comment

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

I just looked at the utils stuff for code quality. Great job breaking things into small helper functions! Some small areas for improvement

utils/MN_util.py Outdated Show resolved Hide resolved
utils/MN_util.py Outdated Show resolved Hide resolved
utils/MN_util.py Outdated Show resolved Hide resolved
utils/MN_util.py Outdated Show resolved Hide resolved
utils/README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@averyschoen averyschoen left a comment

Choose a reason for hiding this comment

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

Great work! I've left in-line comments to address. Additionally, the branch is failing linter check so please address that.

utils/MN_util.py Show resolved Hide resolved
utils/MN_util.py Outdated Show resolved Hide resolved
utils/MN_util.py Outdated Show resolved Hide resolved
utils/MN_util.py Outdated Show resolved Hide resolved
utils/MN_util.py Outdated Show resolved Hide resolved
utils/README.md Show resolved Hide resolved
Copy link

review-notebook-app bot commented Oct 31, 2023

View / edit / reply to this conversation on ReviewNB

averyschoen commented on 2023-10-31T15:23:28Z
----------------------------------------------------------------

inclear why RegNumb and DonationYear are floats and not ints


Copy link

review-notebook-app bot commented Oct 31, 2023

View / edit / reply to this conversation on ReviewNB

averyschoen commented on 2023-10-31T15:23:29Z
----------------------------------------------------------------

add a % here as well


Copy link

review-notebook-app bot commented Oct 31, 2023

View / edit / reply to this conversation on ReviewNB

averyschoen commented on 2023-10-31T15:23:30Z
----------------------------------------------------------------

we will need this at some point in the statecleaner class, write a function to drop the non-classifiable data


Copy link

review-notebook-app bot commented Oct 31, 2023

View / edit / reply to this conversation on ReviewNB

averyschoen commented on 2023-10-31T15:23:31Z
----------------------------------------------------------------

It does not seem like you are using this anymore, delete it if this is true


Copy link

review-notebook-app bot commented Oct 31, 2023

View / edit / reply to this conversation on ReviewNB

averyschoen commented on 2023-10-31T15:23:32Z
----------------------------------------------------------------

if it is going to a PCF show the name of it


Copy link

review-notebook-app bot commented Oct 31, 2023

View / edit / reply to this conversation on ReviewNB

averyschoen commented on 2023-10-31T15:23:33Z
----------------------------------------------------------------

just show the plots of 2018->


Copy link

review-notebook-app bot commented Oct 31, 2023

View / edit / reply to this conversation on ReviewNB

averyschoen commented on 2023-10-31T15:23:34Z
----------------------------------------------------------------

put a space in the legend title


Copy link

review-notebook-app bot commented Oct 31, 2023

View / edit / reply to this conversation on ReviewNB

averyschoen commented on 2023-10-31T15:23:35Z
----------------------------------------------------------------

put a space in the legend titles


notebooks/MN_EDA.ipynb Show resolved Hide resolved
notebooks/MN_EDA.ipynb Show resolved Hide resolved
notebooks/MN_EDA.ipynb Show resolved Hide resolved
notebooks/MN_EDA.ipynb Show resolved Hide resolved
notebooks/MN_EDA.ipynb Show resolved Hide resolved
Copy link
Collaborator

@averyschoen averyschoen left a comment

Choose a reason for hiding this comment

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

Nice work -- in addition to fixing linter I left a few more in-line comments on the EDA. please combine the MN_exp eda into the MN_eda file and remove the mn_exp_eda file from the repo

Copy link
Collaborator

@averyschoen averyschoen left a comment

Choose a reason for hiding this comment

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

We will take a look tomorrow at the linter check

utils/MN_util.py Show resolved Hide resolved
utils/MN_util.py Show resolved Hide resolved
utils/MN_util.py Show resolved Hide resolved
@averyschoen averyschoen merged commit 704e468 into dev Nov 27, 2023
1 check passed
@averyschoen averyschoen deleted the w4_MN_CompleteData_EDA branch November 27, 2023 20:04
trevorspreadbury pushed a commit that referenced this pull request Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants