TargetMeanDiscretiser: sorts variables in bins and replaces bins by target mean value #419
Conversation
|
I was initially thinking that this class would be similar to ArbitraryDiscretiser where the user passes the desired bins. However, I'm now realizing we may want to incorporate the EqualWidthDiscretiser and EqualFrequencyDiscretiser. What are your thoughts? |
|
hi @solegalli, With regards to implementation, are you envisioning the following order of operations or something of the sort?
In the case of multiple variables, the code would iterate through the numeric variables while performing the abovementioned operations. |
There was a problem hiding this comment.
Hi @Morgan-Sell
thank you for getting started with this class.
The implementation looks good to me. I think you kind of answered your own questions, right? or is there anything else I can add?
Main thing, for the first implementation I would not include the arbitrary discretizer. The rest looks good.
Thank you!
| variables: Union[None, int, str, List[Union[str, int]]] = None, | ||
| bins: int = 5, | ||
| strategy: str = "equal_frequency", | ||
| binning_dict: Dict[Union[str, int], List[Union[str, int]]] = None, |
There was a problem hiding this comment.
we do not need this parameter
There was a problem hiding this comment.
Are you referring to binning_dict? Yes, makes sense, given that we're removing the ArbitraryDiscretiser.
| binning_dict: Dict[Union[str, int], List[Union[str, int]]] = None, | ||
| errors: str = "ignore", | ||
| ) -> None: | ||
| # TODO: do we include ArbitraryDiscretiser? |
There was a problem hiding this comment.
I would prefer to just use the equal bin or equal frequency only.
There was a problem hiding this comment.
I'm assuming equal bin is analogous to equal width.
Morgan-Sell
force-pushed
the
target_mean_discretiser
branch
from
7173bbe
to
b6fac50
Compare
|
hi @solegalli, I'm developing the tests for TargetMeanDiscretiser(). I see that the team has made substantial edits to the unit tests. I'm using the EqualFrequencyDiscretiser and EqualWidthDiscretiser tests as guides. Both classes have the following test: This seems to check whether the transformed variable Gracias! |
|
I'm creating the TargetMeanDiscretiser unit tests. I mentioned in an earlier post that I was using the EqualWidthDiscretiser and EqualFrequencyDiscretiser unit tests as guides. However, I think those unit tests do not reflect our most-recent practices, e.g. Gracias! |
| binning_dict: Dict[Union[str, int], List[Union[str, int]]] = None, | ||
| errors: str = "ignore", | ||
| ) -> None: | ||
| # TODO: do we include ArbitraryDiscretiser? |
|
which line of code are you referring to? |
There was a problem hiding this comment.
Hi @Morgan-Sell
Sorry, late response. I added a few comments here and there, minor. We do have the target mean functionality already, no need to code it again.
Basically, we need a pipeline with the discretizer returning the bins as object, followed by the TargetEncoder.
The discretisers tests in general are a bit poor, I agree with you. This is some legacy code. They are some of our first transformers.
Having said this, most tests would be done out-of-the box already by adding the new transformer to the test_check_estimators.py which you have done already.
So for the new tests, I would just ensure that the transformer outputs the expected result.
I would say, have a look at the tests for the TargetMeanRegressor and TargetMeanClassifier. |
|
Soy un boludo! I misinterpreted the instructions, which are in the "TargetMeanDiscretiser". Consequently, I created the Let me know about adding the ArbitraryDiscretiser(). I'll update the test pronto! |
There was a problem hiding this comment.
Hi @Morgan-Sell
This is looking really good thank you.
The tests are failing, any idea which ones and how to fix it?
Also, do you run isort and flake8 on the new files? that would order the imports at the top and tell you which ones are unused, we need to remove to pass the code style tests.
Next steps, documentation! hurray!
We need to expand the docstrings here, and then add the transformer in docs/index and also within the api and user_guide folders, in the last one with a nice demo.
I guess you can take some inspiration from the MeanEncoder's documentation?
| ("discretiser", self._make_discretiser()), | ||
| ("encoder", MeanEncoder( | ||
| variables=self.variables_numerical_, | ||
| ignore_format=True) |
There was a problem hiding this comment.
this param should be False.
There was a problem hiding this comment.
I thought ignore_format should be True to encode the integers that represent the bins created by the discretiser. Otherwise, the transformer throws an error because the variables that are being encoded are not strings.
There was a problem hiding this comment.
That also works. I think it is safest to tell the discretiser to return object variables (return_object=True) and then use the default parameters of the encoder.
|
hola @solegalli, I worked on the documentation (still need to review). I've added the class to multiple Also, the new testing methodology is really cool! I'm still unclear exactly how it operates though. Maybe we can discuss it sometime? Abrazo! |
You need to add the transformer to this file: https://github.com/feature-engine/feature_engine/blob/main/docs/user_guide/discretisation/index.rst I am off on hols since Friday, maybe we can do it on my return? Cheers |
| X, y = check_X_y(X, y) | ||
|
|
||
| # identify numerical variables | ||
| self.variables_numerical_ = _find_or_check_numerical_variables( |
There was a problem hiding this comment.
the attribute should be self.variables_
that should resolve the test failing
|
One of the common tests is failing with this error:
|
|
Gracias, @solegalli! Enjoy your vacay! And, yes, it would be great to review the tests when you return ;) |
|
Did I completely forget about this PR? Looks like it is more or less good to go? |
|
Ok, I went through this quickly, the docstrings still need to be added. The idea of this transformer was to perform discretization, and then replace the values by the mean of the target Now this functionality could be achieved already by combining the equal-frequency or equal-width discretizer with the MeanEncoder in a pipeline. So I am having second thoughts as to whether we should create this class, when it is possible to obtain the same result with the transformers that we already have. Would you mind if we keep this PR on hold? Apologies :/ |
|
hi @solegalli, I'll pause on editing the docstrings. How did you envision this class to be different than combining the equal-frequency or equal-width discretizer with the MeanEncoder in a pipeline? I thought algorithmically this was the purpose. I guess one way to think of this class as a tool in the feature-engine toolbox that a user may not be aware of until reading the feature-engine docs, assuming people read the documentation in depth ;) |
|
Hi @Morgan-Sell There is a paper in the KDD 2009 data science competition, where the authors discretize all numerical variables, and then replace their values by the target mean. And they use this as predictions to basically select features. For categorical variables, they replace the categories by the target mean. And they proceed to select features in the same way. So the MeanEncoder and the TargetMeanDiscretizer aim to do the same thing: replace the categories by the target mean and sort the variables into intervals and then replace the intevals by the target mean. This is where my thinking came from. Most users would not be aware of this possibility, because well, they don't know all the literature and probably don't have time to catch up. So having the class ready, is helpful, in the sense that it makes the transformation "visible". On the other hand, it is more work for us, given that the encoding could be done already with the classes that already exist. So yeah, I am in 2 minds. What do you think? |
|
I made a PR with some changes here: only 2 things remain doing and then we are good to go! |
|
@Morgan-Sell this in reply to whether or not we should move forward. I am still in 2 minds. What's your view? |
|
I'm leaning towards creating the transformer. I see Python packages as toolboxes and like to consider the novice craftsman - e.g. me - when designing the tools. Like you said, people are going to open the toolbox and be amzed by the collection of our wonderful tools. It's unlikely for a novice to make the connection between the numerical discretizer and mean encoder. Also, the use may be unaware of sklearn's Pipeline class. I think we've already done most of the work. Once created, how much maintenance is required for each class? I'm assuming it's not much but could depend on the class/transformer's complexity. I guess this class will need to be updated whenever changes are made to the numerical discretizers or mean encoder. I'm still leaning towards creating the class for the newbies as I'm one of them ;) |
|
Sounds good, Let's finish the IV selector, and then we come back to this one. In the meantime, we sleep on the idea a bit longer :p |
Closes #394.
The transformer accepts a dictionary that defines how numeric variables will be discretized/organized into bins. The transformer calculates and returns the average for the respective bins.