Skip to content

Conversation

EssamWisam
Copy link
Collaborator

It turns out that most encoder or transformers implemented in here were subject to the bug mentioned in FluxML/MLJFlux.jl#275 . This PR attempts to fix the output types for all the encoder/transformer methods implemented in this package.

In doing so, adherence to the following performance tip in the Julia doc was considered:

If you cannot avoid containers with abstract value types, it is sometimes better to parametrize with Any to avoid runtime type checking. E.g. IdDict{Any, Any} performs better than IdDict{Type, Vector}

which can be observed in the target, frequency and ordinal encoder changes. It could be helpful to consider changes commit by commit.

@EssamWisam
Copy link
Collaborator Author

Merging this should close #20

Copy link
Member

@ablaom ablaom left a comment

Choose a reason for hiding this comment

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

Thanks for catching this and adding all the new tests.

Based on normalize, we may want integers or floats so let's give this flexibility
Copy link
Member

@ablaom ablaom 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 to go, thanks again.

@EssamWisam EssamWisam merged commit d9d28ed into main May 15, 2025
3 checks passed
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