Pytorch-lightning: ModelCheckpoint automatic mode selection is misleading

[wolterlw]

🐛 Bug

https://github.com/PyTorchLightning/pytorch-lightning/blob/2d78d9b84af96c6fe06a4b2e4a27754a275db460/pytorch_lightning/callbacks/model_checkpoint.py#L299-L305

This behaviour makes a strong assumption that any non-accuracy/fmeasure metric is a loss.

To Reproduce

I use a metric called val_pck_0.15, which stands for Percentage of Correct Keypoints (popular metric in pose estimation). It was misclassified and time was wasted.

Expected behavior

There should either be a dictionary with predefined metric names and corresponding configs or IMHO there should be no automatic mode selection, just a MisconfigurationError if mode is not provided.

[github-actions[bot]]

Hi! thanks for your contribution!, great first issue!

[carmocca]

I agree that the auto logic is too simple. However, having a dictionary of predefined metrics may be too complicated and still might lead to confusion for users with not-so-simple monitor names.

I second deprecating automatic mode selection. Users already have to set the monitor when they create a ModelCheckpoint (asumming they want to monitor a quantity), it is natural to have them also set the mode.

[ananthsub]

+1 the mode should just be min or max

[Borda]

@carmocca @wolterlw we can deprecate the auto od do you have any suggestion how the automatic should perform in a more clever way?
cc: @PyTorchLightning/core-contributors

[wolterlw]

One could add a flag to the Metric class and go from there, but that seems unnecessarily complex.
I agree with @carmocca that making mode a required parameter is short, explicit and simple.

[awaelchli]

I vote for default mode="min".

[williamFalcon]

i agree with max or min as options with default as min

[Borda]

One could add a flag to the Metric class and go from there, but that seems unnecessarily complex.
I agree with @carmocca that making mode a required parameter is short, explicit and simple.

mind send a PR :]

[ydcjeff]

Then we could also do the same for EarlyStopping as well?