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

Fix column_name and null generation #337

Conversation

ksneab7
Copy link
Contributor

@ksneab7 ksneab7 commented Sep 27, 2023

No description provided.

Copy link
Contributor

@taylorfturner taylorfturner left a comment

Choose a reason for hiding this comment

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

let's validate tests

@taylorfturner taylorfturner added the bug Something isn't working label Sep 27, 2023
tests/test_generators.py Outdated Show resolved Hide resolved
@taylorfturner taylorfturner changed the base branch from main to feature/bug-fix September 27, 2023 17:00
@ksneab7 ksneab7 force-pushed the bug_fix_for_column_header_issue branch from 35d055f to 8d65506 Compare September 27, 2023 17:02
taylorfturner
taylorfturner previously approved these changes Sep 27, 2023
@taylorfturner taylorfturner changed the title fix for column name exclusion bug Fix column_name and null generation Sep 27, 2023
@@ -0,0 +1,14 @@
"""Contains a random Null generator."""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function is used to generate fully null columns


col_["rng"] = self.rng
col_["num_rows"] = num_samples
if generator_name:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed in place of the removed warning, as we account for a fully nulled column

Comment on lines 148 to 149
else:
generator_name = "null_generator"
Copy link
Contributor

Choose a reason for hiding this comment

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

might do an elif here for readability

Comment on lines 169 to +172
else:
if col_["order"] is not None:
if (not generator_name == "null_generator") and col_[
"order"
] is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

just an else here (only way it gets is if generator_name not in the dictionary keys) and just logging.warning)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The warning above is not needed anymore just because of the null columns now being accounted for.

Copy link

@danielbarcklow danielbarcklow left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@taylorfturner taylorfturner left a comment

Choose a reason for hiding this comment

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

LGTM

@taylorfturner taylorfturner merged commit df7c401 into capitalone:feature/bug-fix Sep 27, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants