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

[DO NOT MERGE] Code Review #1

Closed
wants to merge 3 commits into from
Closed

Conversation

alanhdu
Copy link

@alanhdu alanhdu commented Mar 23, 2016

I've exported your notebook to go through some code review.

density.ipynb has my combined changes, while density.py is your original code (and is where my comments are)

In general, try to adhere to pep8 for formatting.

import pandas as pd
import matplotlib.pyplot as plt
import statsmodels.api as sm
import seaborn as sb
Copy link
Author

Choose a reason for hiding this comment

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

There are a lot of unused imports. Some of them (e.g. np) are fine for exploration, but I'm not sure why we need os

Copy link
Author

Choose a reason for hiding this comment

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

The standard abbreviation for seaborn is

import seaborn as sns

and not

import seaborn as sb

(Don't ask me why).


def floor_plot(*args, **kwargs):
group = kwargs.pop('group', None)
group_word = {None:"", "week":"Weekly ", "day":"Daily "}
Copy link
Author

Choose a reason for hiding this comment

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

You want spaces between the colon and the value:

group_word = {None: "", "week": "Weekly", ...}
                   ^           ^

@alanhdu alanhdu closed this Apr 8, 2016
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.

None yet

1 participant