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

Create BaseDropOriginalMixin Class #248

Merged
merged 6 commits into from
May 10, 2024
Merged

Conversation

Chip2916
Copy link
Collaborator

@Chip2916 Chip2916 commented May 9, 2024

As per ticket here

  • Added mixin class for transformers that take 'drop_original' argument.
  • Added this class to mixin script as per weight class mixin script created during test refactor.
  • Added associated tests to the base tests file rather than creating a new script specifically for this (as per weight class mixin also)
  • Updated transformers to inherit from new mixin class where required
  • Updated test files to inherit from new mixin test class where required and where refactor has taken place

@Chip2916 Chip2916 closed this May 9, 2024
@Chip2916
Copy link
Collaborator Author

Chip2916 commented May 9, 2024

Linting errors caused by resolution of merge conflicts, will set up another

@Chip2916 Chip2916 reopened this May 9, 2024
@Chip2916 Chip2916 marked this pull request as ready for review May 9, 2024 08:43
Copy link
Contributor

@davidhopkinson26 davidhopkinson26 left a comment

Choose a reason for hiding this comment

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

Looks good, couple of suggestions I would like to see addressed but both minor.

@@ -1246,9 +1247,12 @@ def transform(self, X: pd.DataFrame) -> pd.DataFrame:
)

# Drop original columns
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the comment here could do with extending with ' if self.drop_original is true' as the functionality is not obvious without reading the called function.

assert ("a" in df_transformed.columns.to_numpy()) and (
"b" in df_transformed.columns.to_numpy()
), "original columns not kept"

Copy link
Contributor

Choose a reason for hiding this comment

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

S - ideally would also have a test that other columns in X are unaffected when dropping columns addressed by the transformer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for reviewing David, have pushed recommend changes :)

Copy link
Contributor

@davidhopkinson26 davidhopkinson26 left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks 🙏

@Chip2916 Chip2916 merged commit 6c36197 into main May 10, 2024
12 checks passed
@Chip2916 Chip2916 deleted the feature/drop_original_mixin branch May 10, 2024 12:57
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.

DropOriginalMixin Class
2 participants