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

feat(python,rust): add drop_first parameter for to_dummies (issue #8246) #9143

Merged
merged 2 commits into from
Jun 23, 2023
Merged

feat(python,rust): add drop_first parameter for to_dummies (issue #8246) #9143

merged 2 commits into from
Jun 23, 2023

Conversation

EdmundsEcho
Copy link
Contributor

@EdmundsEcho EdmundsEcho commented May 31, 2023

(Closes #8246).

The fix add the "drop_first" parameter to the to_dummies function. See issue #8246: "to_dummies implementation may be incorrect". The changes were made in-line with py/pandas. The parameter is a bool with a default value false. The default value is False; consistent with py/pandas.

I updated the current tests where applicable by including the additional parameter set to false. I did not create new tests with the parameter set to true (or True in py).

However, I did test the changes in the rust code on my local machine. The reason for not adding a new test was only because several unrelated tests were not passing And I was not familiar with the testing structure.

@avimallu
Copy link
Contributor

avimallu commented Jun 1, 2023

You might need to rename the issue as feat(python,rust): add drop_first parameter for to_dummies or something similar. Right now the title isn't very descriptive.

@EdmundsEcho EdmundsEcho changed the title issue #8246; completed tested fix feat(python,rust): add drop_first parameter for to_dummies (issue #8246) Jun 2, 2023
@github-actions github-actions bot added enhancement New feature or an improvement of an existing feature python Related to Python Polars rust Related to Rust Polars labels Jun 2, 2023
@alexander-beedie
Copy link
Collaborator

alexander-beedie commented Jun 21, 2023

Hi @EdmundsEcho! Thanks for the contribution; I've addressed the lint errors, added some test coverage and made a small fix - if you'd like to get this into our next release, I can push these updates onto your branch (here) so we can finish it off? (Though I think you'd have to allow me to do so: see the "Allow edits from maintainers" checkbox) ;)

@avimallu
Copy link
Contributor

+1 on getting this to the next release, a very helpful argument to have!

@EdmundsEcho
Copy link
Contributor Author

@alexander-beedie I had to invite you to be a writing collaborator to the project. This is a bug with github hosted under an organization (Lucivia). Let me know if you can't make the push happen. I'll see what I can do to integrate the changes.

@alexander-beedie
Copy link
Collaborator

alexander-beedie commented Jun 22, 2023

I had to invite you to be a writing collaborator to the project.
Let me know if you can't make the push happen.

Great, thanks! Think this should have got it... let's see how the tests go ;)

@avimallu
Copy link
Contributor

avimallu commented Jun 22, 2023

This should close #8246 and #9483, to keep track.

@alexander-beedie
Copy link
Collaborator

This should close #8246 and #9483, to keep track.

Thanks :)

Copy link
Member

@ritchie46 ritchie46 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 everybody.

@ritchie46 ritchie46 merged commit 373a99a into pola-rs:main Jun 23, 2023
@alexander-beedie alexander-beedie deleted the new-dummy-option branch June 23, 2023 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or an improvement of an existing feature python Related to Python Polars rust Related to Rust Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

to_dummies implementation may be incorrect
4 participants