Fix small bug in _get_params_callbacks

[BlackHC]

Swap the order of default callbacks and current callbacks passed to itertools.chain in _get_params_callbacks.

We want the default callbacks first so that the current callbacks can override them. The fixed code now matches the order in _yield_callbacks.

[githubnemo]

Thanks for the PR, the change seems reasonable :)

It would be perfect if you could add a test that tests the desired effect (i.e. overriding of a default callback)!

[BenjaminBossan]

Thanks for the fix. As githubnemo mentioned, it would be nice to have a test (also, an entry to CHANGES.md).

We want the default callbacks first so that the current callbacks can override them.

For my understanding, is the idea for the user to have a callback with the same name as one of the default callbacks (say, 'print_log') in order to override it? I haven't thought about achieving it that way. Do you have an example where this is relevant?

[BenjaminBossan]

@BlackHC Would you be willing to address the point? Otherwise, we would probably create a separate PR (still crediting you).

[BlackHC]

I will have a look this week. Sorry for the delays. I was traveling for a conference and took some time off.

Yes, the idea was to be able to override default callbacks as well and to also match the existing behavior in other methods (to avoid subtle bugs in the future).

I'll try to write tests for this that cover both _get_params_callbacks and _yield_callbacks hopefully to make sure that they stay in sync.

Best wishes,
Andreas

[BenjaminBossan]

Fantastic, thanks @BlackHC. No time pressure, I just wasn't sure if you were still interested in working on this.

[BenjaminBossan]

@BlackHC a friendly reminder.

[BlackHC]

I have taken another look at this.

Is the canonical way to disable a callback similar to: net_cls(module_cls, callbacks__print_log=None)? (And similar to override a default callback we can specify callbacks__print_log=CustomPrintLog?

Writing a test, I realized that _yield_callbacks, _get_params_callbacks and initialize_callbacks all interact very subtly and it is not clear that my change is improving much. If the above way of overriding default callbacks works, maybe the docs could be updated to mention that and I can drop this change.

[BenjaminBossan]

Is the canonical way to disable a callback similar to: net_cls(module_cls, callbacks__print_log=None)? (And similar to override a default callback we can specify callbacks__print_log=CustomPrintLog?

This is using the same idea as in sklearn Pipelines where individual steps can be skipped by setting them to None:

A step’s estimator may be replaced entirely by setting the parameter with its name to another estimator, or a transformer removed by setting it to 'passthrough' or None.

(docs)

Although it seems that we should also allow "passthrough" as a way to skip a callback.

it is not clear that my change is improving much

Hmm, I was taking a stab and I think that the initial concern, namely:

We want the default callbacks first so that the current callbacks can override them.

is not really a concern, since skorch will raise an error if a callback name is duplicated. Honestly, it's something that I personally never had a use for, so I forgot about it. I guess we could still argue that the proposed change is "more correct" and write a test that checks the order of the returned params, but I agree it's not super useful.

maybe the docs could be updated to mention that and I can drop this change

Yes indeed. Is that something you're interested in working on?