Validation data generation, removal of loss function overrides

[kaueltzen]

Resolves #228 .

Overview

• Replaces train_test_split with explicit generation of validation data that is reproducible, shuffled and stratified (if classification tasks are present)
  • as in Fix bootstrap fixed random state #232 no explicit determination of classification targets for stratification, uses first target instead
• Removes override of loss function for classification tasks in fit of MODNetModel

[kaueltzen]

Hi @ppdebreuck I have a question regarding this comment:
#228 (comment)

Do you think it would make sense to enable the passing of a loss function for classification tasks in evaluate of MODNetModel?
Or would you prefer keeping -ROC-AUC (e.g., because it's easier to interpret than cross entropy losses)?

[ppdebreuck]

Thanks @kaueltzen ! Looks good, there just might be something off in the the stratified split, please let me know if you agree.

[ppdebreuck]

I prefer to have this function under modnet.utils and import it as from modnet.utils import generate_shuffled_and_stratified_val_split. This will avoid having it in __all__

[ppdebreuck]

This is most likely wrong. It's picking the first row, but we need the first column: ycv = y[:,0]

[kaueltzen]

Hi @ppdebreuck thanks for pointing that out!
It was written for MODNetModel.fit() to handle a list of props from targets_groups, but outside of vanilla.py / generate_shuffled_and_stratified_val_data the targets are indeed not correctly handled, I will change it.

[ppdebreuck]

this can simply become range(len(ycv))

[ppdebreuck]

Edit: let's do range(len(y)), so it's compatible with my last comment.

[ppdebreuck]

Stratifying a multilabel case is a bit tricky, and probably you don't need it ? So we can skip it: ycv=None

[ppdebreuck]

This doesn't seem correct ? y[train_idx] should work, right?. It would pick the wrong columns now.

[kaueltzen]

The y of MODNetModel.fit() and of generate_shuffled_and_stratified_val_data has the first two dimensions (n_target_groups, n_samples) while the y of generate_shuffled_and_stratified_val_split that is used elsewhere (data.df_targets.values) has the first 2 dimensions (n_samples, n_targets).

[ppdebreuck]

Hi @ppdebreuck I have a question regarding this comment: #228 (comment)

Do you think it would make sense to enable the passing of a loss function for classification tasks in evaluate of MODNetModel? Or would you prefer keeping -ROC-AUC (e.g., because it's easier to interpret than cross entropy losses)?

No problem with me, as you can put ROC_AUC as default metric to keep current behavior, while adding flexibility if you need to change it :) (I would put it in a separate PR)

[ppdebreuck]

Thanks for the modifications @kaueltzen, I think this can be merged ?