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.

Sign up for GitHub

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

Jump to bottom

Issue 86: Error merging extensions on modeler.CPA #88

Merged
merged 11 commits into from
Feb 13, 2019
Merged

Issue 86: Error merging extensions on modeler.CPA #88

merged 11 commits into from
Feb 13, 2019

Conversation

ManuelAlvarezC
Copy link
Contributor

@ManuelAlvarezC ManuelAlvarezC commented Feb 7, 2019 •
edited
Loading

This PR contain the changes made in order to resolve #86:

  • Little rework of examples/demo.py
  • Make modeler.CPA work when index of parent table is not numerical.
  • Make modeler.CPA work when name of foreign_key and primary_key are not the same.
  • Make modeler.impute_table only iterate over columns with nulls, and only computing mean on numerical columns.
  • Test for the aforementioned changes.
  • During the changes above, a bug, where an exception was created, but not raised, was found, that has been fixed too.

csala
csala approved these changes Feb 12, 2019
View reviewed changes
Copy link
Contributor

@csala csala left a comment

Choose a reason for hiding this comment

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

I put some comments about an alternative to boto3, but overall it looks good.

examples/utils.py Outdated Show resolved Hide resolved
examples/utils.py Outdated Show resolved Hide resolved
sdv/modeler.py Outdated Show resolved Hide resolved
@ManuelAlvarezC ManuelAlvarezC merged commit 26ba9db into master Feb 13, 2019
@ManuelAlvarezC ManuelAlvarezC deleted the 86_error_merging_extensions branch February 13, 2019 12:06
JonathanDZiegler pushed a commit to JonathanDZiegler/SDV that referenced this pull request Feb 7, 2022
@csala
Update release date (sdv-dev#88)
a464385
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@csala csala csala approved these changes

@kveerama kveerama Awaiting requested review from kveerama

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Error when merging extension in modeler CPA when running examples
2 participants
@ManuelAlvarezC @csala