feat: add handle_zero option in ZeroInflatedRegressor estimator

[mkalimeri]

Hi, please review my code for issue 480 - Run with given regressor instead of raising warning in ZeroInflatedRegressor

There are changes in two files

1. zero_inflated_regressor.py: As explained in my comment, I approached this issue in the following way

• I added a flag 'handle_error', which can take two values: 'error' and 'ignore'. If the user chooses 'error', then, in the case that the train set output only consists of zeros, a ValueError is thrown. If the user chooses 'ignore' and if all the train set outputs are zero, the regressor fits all the train set (the flag handle_error='ignore' is taken into consideration only if there are no non-zero targets). So, when there are only zero targets and the user chooses handle_error='ignore', the regressor will train on the whole dataset, but if there are non-zero targets, the functionality remains as is (ignore lines with zero target)

• The handle_error='error' as default, so the user should actively choose to ignore the fact that the train dataset contains only zero targets.

2. test_zero_inflated_regressor.py: Added unit tests for the new functionality

Please let me know what you think and if you have any suggestions about how to improve the code!

[koaning]

Before I dive deeper, could you check and confirm why the tests fail? Just want to make sure it's not an oversight.

[mkalimeri]

Hi Vincent, sorry about that, I was just looking at this. There is a mismatch in the message of the ValueError thrown (see below). Best regards, Maria [image: Screenshot 2024-11-07 at 14.04.12.png]

…

On Thu, Nov 7, 2024 at 1:59 PM vincent d warmerdam ***@***.***> wrote: Before I dive deeper, could you check and confirm why the tests fail? Just want to make sure it's not an oversight. — Reply to this email directly, view it on GitHub <#714 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ASO4IW7WEXBM6RKNC6FRFODZ7NP2NAVCNFSM6AAAAABRLD3KROVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINRSGE3TQMJYGU> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[mkalimeri]

Hi again, sorry about that... I fixed it locally, shall I send another PR? BR, Maria On Thu, Nov 7, 2024 at 2:04 PM Maria Kalimeri ***@***.***> wrote:

…

Hi Vincent, sorry about that, I was just looking at this. There is a mismatch in the message of the ValueError thrown (see below). Best regards, Maria [image: Screenshot 2024-11-07 at 14.04.12.png] On Thu, Nov 7, 2024 at 1:59 PM vincent d warmerdam < ***@***.***> wrote: > Before I dive deeper, could you check and confirm why the tests fail? > Just want to make sure it's not an oversight. > > — > Reply to this email directly, view it on GitHub > <#714 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/ASO4IW7WEXBM6RKNC6FRFODZ7NP2NAVCNFSM6AAAAABRLD3KROVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINRSGE3TQMJYGU> > . > You are receiving this because you authored the thread.Message ID: > ***@***.***> >

[FBruzzesi]

Hey @mkalimeri , thanks for the contribution. Code is fine, I left a few comments mostly for documentation and error messages.

Additionally, could you please add a test that is not raising when training with y consisting of zeros only? i.e. with handle_zero='ignore'?

I also took the liberty to change the PR title, to have some more context from the title directly

[mkalimeri]

Thank you for the review, I accepted the edits as suggested and will add the unit test shortly

[FBruzzesi]

Hey @mkalimeri thanks a ton for the effort on this 🚀

I committed a couple of changes to move handle_zero argument to the estimator initialization instead of its fit method. Aside that your logic remained untouched!

We should be ready to merge 🙌🏼

[mkalimeri]

Hi @FBruzzesi, thank you for improving the code and merging. This was fun! :-)