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

ENH Warn when expanding columns with many categories #26

Conversation

stephen-hoover
Copy link
Contributor

@stephen-hoover stephen-hoover commented Mar 1, 2018

Users sometimes forget to exclude columns with large numbers of categories (for example, a dataset index, or a free text response field). Give them a warning so that it's easier to diagnose if this causes errors later. The threshold is a class attribute to allow users to modify it if desired (but it's not a constructor parameter because they shouldn't want to modify it often, if ever).

Closes #25 .

Users sometimes forget to exclude columns with large numbers of categories (for example, a dataset index, or a free text response field). Give them a warning so that it's easier to diagnose if this causes errors later. The threshold is a class attribute to allow users to modify it if desired (but it's not a constructor parameter because they shouldn't want to modify it often, if ever).
@beckermr
Copy link
Contributor

beckermr commented Mar 2, 2018

FTR, I don't really want to see this warning. Users are responsible for knowing what is in their data.

@stephen-hoover
Copy link
Contributor Author

I've seen more than one case where this warning would have saved significant amounts of time.

@beckermr
Copy link
Contributor

beckermr commented Mar 2, 2018

Yeah. People inspecting their data first would have done the same trick. (shrug)

Copy link
Contributor

@elsander elsander left a comment

Choose a reason for hiding this comment

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

Good idea on this change-- I've debugged this issue for users several times, and a warning will make it easier for them to identify the problem themselves.

LGTM!

@elsander elsander assigned stephen-hoover and unassigned elsander Mar 2, 2018
@stephen-hoover stephen-hoover merged commit 4c84e9c into civisanalytics:master Mar 2, 2018
@stephen-hoover stephen-hoover deleted the warn-for-lots-of-expansions branch March 2, 2018 16:46
@stephen-hoover stephen-hoover added this to the v0.2.0 milestone Mar 5, 2018
@stephen-hoover stephen-hoover modified the milestones: v0.2.0, v0.1.7 Mar 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants