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

exclude_columns does not work for ColumnsBasedPipelineStage #100

Closed
jjlee88 opened this issue May 12, 2022 · 4 comments
Closed

exclude_columns does not work for ColumnsBasedPipelineStage #100

jjlee88 opened this issue May 12, 2022 · 4 comments
Assignees
Labels

Comments

@jjlee88
Copy link

jjlee88 commented May 12, 2022

When setting exclude_columns as a list of strings in the constructor of ColumnsBasedPipelineStage the member variable self._exclude_columns is set to a tuple instead of the passed in list. This leads to errors with __get_cols_by_arg which ends up returning the tuple and not the list itself.

Code where the issue appears to be

self._exclude_columns = self._interpret_columns_param(

The line which seems incorrect is linked above. _interpret_columns_param returns a tuple, so we should be assigning the individual components of the tuple, in that line, as opposed to assigning the entire return value to 'self._exclude_columns'; something like

self._exclude_columns, _ = self._interpret_columns_param(...

@shaypal5
Copy link
Collaborator

shaypal5 commented May 12, 2022

Right you are! Thank you for catching this!

I'll fix this and add a test.

It shouldn't be discarded, by the way. The second element is an str representation of the columns to be discarded. It will be used to construct the pipeline description string.

@shaypal5
Copy link
Collaborator

Done. The new test now catches this error:
https://github.com/pdpipe/pdpipe/actions/runs/2313073586

And the same test passes for the fix:
https://github.com/pdpipe/pdpipe/actions/runs/2313093564

@shaypal5 shaypal5 added the bug label May 12, 2022
@shaypal5 shaypal5 self-assigned this May 12, 2022
@jjlee88
Copy link
Author

jjlee88 commented May 12, 2022

Thank you for fixing it and thank you for writing this library!

@shaypal5
Copy link
Collaborator

No problem! I wrote it for fun, and it's an immense pleasure to have other people find use for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants