feat: adds optional `categorical` paramater as option to onehot trans…

[rajatrc1705]

…form

fixes #158

Default Behavior (categorical is true by default)

Behavior When Categorical is set to false

[juliohm]

@rajatrc1705 did you have a chance to look into this?

[rajatrc1705]

@juliohm yes I am on it, currently having some trouble with the revertfeat function (failing the tests for onehot.jl), so troubleshooting that part. Will push the changes soon. Apologies if this is taking longer than usual! Will do better for the next contri.

[juliohm]

No worries @rajatrc1705 ! Just wanted to double check you are still with us :)

[rajatrc1705]

I am still working on the revertfeat function, its taking some time, so meanwhile I have pushed the other changes that I have made.

[rajatrc1705]

Also.. do beginners usually struggle so much with their first PRs 😅, it seems I am missing a lot of things. Is there some guideline or something that you recommend I read up before starting any future contributions to the julia organization.

[juliohm]

Also.. do beginners usually struggle so much with their first PRs sweat_smile, it seems I am missing a lot of things. Is there some guideline or something that you recommend I read up before starting any future contributions to the julia organization.

I think you start like this. At each PR you learn something new, this is the way open-source works usually. There is no well-defined recipe and each project maintainer has different standards.

[juliohm]

@rajatrc1705 fixed a couple of issues with the proposed changes. Now we are missing tests for the new default with categorical=true. Can you please add new tests to make sure that the option is working as expected?

[rajatrc1705]

@rajatrc1705 fixed a couple of issues with the proposed changes. Now we are missing tests for the new default with categorical=true. Can you please add new tests to make sure that the option is working as expected?

Yes I will write the new tests and commit the same here.

[juliohm]

The changes here need to be fixed then because old code should work without passing two options.

Em seg., 2 de jan. de 2023 15:40, rajatrc1705 @.***> escreveu:

@rajatrc1705 https://github.com/rajatrc1705 fixed a couple of issues with the proposed changes. Now we are missing tests for the new default with categorical=true. Can you please add new tests to make sure that the option is working as expected?

Yes I will write the new tests and commit the same here.

— Reply to this email directly, view it on GitHub https://github.com/JuliaML/TableTransforms.jl/pull/161#issuecomment-1369141882, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAZQW3OEUJZCCAEBBOZTVYDWQMOKTANCNFSM6AAAAAATE6DSSE . You are receiving this because you were mentioned.Message ID: @.***>

[rajatrc1705]

Yes, for the code which I wrote, OneHot function was working without having to pass two parameters. I tested it with a few examples also.

b=OneHot("a") resulted in categorical being true

[juliohm]

Please share text in comments instead of screenshots. It is very hard to search/copy/paste that way.

Also, try to start discussing what is needed moving forward to make things work instead of discussing the old version that wasn't ideal either.

[rajatrc1705]

Please share text in comments instead of screenshots. It is very hard to search/copy/paste that way.

Also, try to start discussing what is needed moving forward to make things work instead of discussing the old version that wasn't ideal either.

The fix shared by @eliascarv works perfectly well! The tests for the case when categorical = true remain.

• The onehot columns for both categorical=false and categorical=true will be same, only type checking must be carried out in the new tests eg: typeof(column) == typeof(categorical(Bool[]))
• The revertfeat function for OneHot should be able to revert categorical arrays back to their original form, so modifications needed in the revertfeat function

[juliohm]

The fix shared by @eliascarv works perfectly well! The tests for the case when categorical = true remain.

Looking forward to the new commits.

You can press "." on your keyboard at this page and GitHub will open the web editor where you can update the files accordingly.

[juliohm]

Thanks @eliascarv for taking the lead. We are only missing tests now with the new default.

[codecov-commenter]

Codecov Report

Merging #161 (33a5612) into master (a3e8894) will increase coverage by 0.01%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #161      +/-   ##
==========================================
+ Coverage   95.16%   95.17%   +0.01%     
==========================================
  Files          27       27              
  Lines         889      891       +2     
==========================================
+ Hits          846      848       +2     
  Misses         43       43              

Impacted Files	Coverage Δ
src/transforms/onehot.jl	97.50% <100.00%> (+0.13%)	:arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[juliohm]

Thanks @eliascarv ! 💯

[juliohm]

Thanks @rajatrc1705 ! 💯