Closed PaulWestenthanner closed 2 years ago
Hi @nercisla
Thanks for your work. I created a draft pull request just because it is easier and more transparent to make comments and have the tests run. I hope that's fine with you. Also feel free to change the description if you want to
Hi @nercisla
thanks for updating the PR. I think it looks really good now. The only open issue I see is the test that checks if a trivial hierarchy yields the same encoding as no hierarchy.
Do you see any other open topics that need to be fixed before merging?
Really looking good now and I think we can merge next week. Thank you!
I think the trivial hierarchy issue is tested by two tests:
The features we want to move on to are (in order of priority in our opinion):
We think these are "nice to have" extras, which don't need to be included just now.
For info, I have a concern over the mapping. The current code works and gives the right answer, but the mapping is reversed if we want to consider multi-level mapping. So it might be good to consider larger hierarchies and a solution before we merge...
Detail: we currently have a mapping dictionary that goes from the existing column (key) to the higher level mapping (value). But for multi-level, it makes more sense to have the highest levels first in the dictionary. So, we need to consider reversing the map. The mapping won't be so trivial for multi-level in that scenario, but makes more sense for the user.
Joe and I have modified the code to "reverse" the mapping dictionary. This should now follow a more user-friendly version of mapping. Multi-level mapping is also complete and we've included a test.
Great! Thanks for implementing the multilevel hierarchy. Maybe it would be nice to give an example of the hierarchy parameter in the docstrings since this is getting a little complicated (especially with multilevel)
Have added a simple example, since more complex ones might take even more space.
Great!
Thank you very much @nercisla and @joeybricks
The tests are succeeding and I'd consider it ready for the merge. Is this fine with you or is there anything missing?
Very happy to merge!
Fixes #136
Proposed Changes
This pull request implements feature hierarchies in Target Encoders.
Author: @nercisla Current status: Work in Progress