Fit callback in BPR model

[apat1n]

I found fit_callback in ALS implementation, but it can be also useful in BPR model. For example:

def calc_hitrate_callback(model: BayesianPersonalizedRanking, epoch: int) -> dict:
    if (epoch + 1) % VAL_PERIOD != 0:
        return {}
    
    recommendations = prepare_recommendations(model)
    hitrate_list = calc_hitrate(recommendations, val_dataset, K)
    return {f'mean hitrate@{K}': np.mean(hitrate_list).round(5)}

bpr_model.fit(train_dataset, fit_callback=calc_hitrate_callback)

[ita9naiwa]

Hi. Adding callback looks good to me, but
IMHO, I think it would be better to consider those two followings.

• in ALS.fit() fit_callback is not in its argument. (I guess it might be fixed too)
• function fit_callback will look better if it is consistent with https://github.com/benfred/implicit/blob/main/benchmarks/benchmark_als.py here.

[apat1n]

Hi! IMHO, I found it more useful if callback function will have straight access to model and that may be better for evaluation than ALS callback format. And if it would be in BPR.fit(), it could be found in API docs

[ita9naiwa]

I think your idea makes sense. however, it looks not good practice that same function (also sharing same name) has different behaviors in a different module.

So, considering that fit_callback is already implemented and being used for ALS, I think fit_callback should follow the same format as used in benchmark_als.py.

[ita9naiwa]

my one suggestion is that it will be also good if ALS and other recommender models take fit_callback as an argument

[apat1n]

That sounds good, also I kept backward compatibility in ALS model

[ita9naiwa]

Looks good to me.