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

Feature/two column operator #291

Merged
merged 3 commits into from
Sep 4, 2024
Merged

Conversation

Chip2916
Copy link
Collaborator

@Chip2916 Chip2916 commented Sep 3, 2024

Refactored TwoColumnOperator to align with new test design.

Linked issue here

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.

Broadly happy with this. Bit confused by the comment about DFMTransformer so would like clarification there before approving.

tests/base_tests.py Show resolved Hide resolved

self.column1_name = columns[0]
self.column2_name = columns[1]
# Set above call to super as DFMTransformer accepts columns, but this transformer only ever needs one
Copy link
Contributor

Choose a reason for hiding this comment

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

'Set below call'? I'm not quite sure what you mean here, this transformer needs two columns?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hopefully this comment makes it clearer!

@@ -419,7 +420,7 @@ def transform(self, X: pd.DataFrame) -> pd.DataFrame:
msg = f"{self.classname()}: input columns in X must contain only numeric values"
Copy link
Contributor

Choose a reason for hiding this comment

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

N: noticed an error on line 414:

'# call BaseTransformer.transform
X = super(DataFrameMethodTransformer, self).transform(X)'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch!

@davidhopkinson26 davidhopkinson26 merged commit aaafda0 into main Sep 4, 2024
12 checks passed
@davidhopkinson26 davidhopkinson26 deleted the feature/TwoColumnOperator branch September 4, 2024 09:32
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.

2 participants