[Improvement] Modify default value of save_best

[irvingzhang0512]

Fix #599

[irvingzhang0512]

if save_best='auto', then we have to set self.rule manually.

mmaction2/mmaction/core/evaluation/eval_hooks.py

Lines 101 to 126 in 2f7368c

	def _init_rule(self, rule, key_indicator):
	"""Initialize rule, key_indicator, comparison_func, and best score.
	Args:
	rule (str | None): Comparison rule for best score.
	key_indicator (str | None): Key indicator to determine the
	comparison rule.
	"""
	if rule not in self.rule_map and rule is not None:
	raise KeyError(f'rule must be greater, less or None, '
	f'but got {rule}.')
	if rule is None:
	if key_indicator != 'auto':
	if any(key in key_indicator for key in self.greater_keys):
	rule = 'greater'
	elif any(key in key_indicator for key in self.less_keys):
	rule = 'less'
	else:
	raise ValueError(f'Cannot infer the rule for key '
	f'{key_indicator}, thus a specific rule '
	f'must be specified.')
	self.rule = rule
	self.key_indicator = key_indicator
	if self.rule is not None:
	self.compare_func = self.rule_map[self.rule]

I'm a little confused about the codes. It seems that self.rule should not be None at all. Maybe we need to add assert for that self.rule is not None.

@dreamerlin Any suggestions?

[dreamerlin]

I guess you mean the case rule=None & save_best='auto. Actually in this case, self._init_rule will be called twice.

At first time:

mmaction2/mmaction/core/evaluation/eval_hooks.py

Lines 97 to 99 in 2f7368c

	if self.save_best is not None:
	self.best_ckpt_path = None
	self._init_rule(rule, self.save_best)

The result is: self.rule=None, self.key_indicator='auto', self.compare_func is not set

At second time:

mmaction2/mmaction/core/evaluation/eval_hooks.py

Lines 245 to 247 in 2f7368c

	if self.key_indicator == 'auto':
	# infer from eval_results
	self._init_rule(self.rule, list(eval_res.keys())[0])

This time, the key_indicator is not 'auto', and then the result: self.rule is not None but between 'greater' and 'less', self.key_indicator is not 'auto' and se;f.compare_func is determined.

[dreamerlin]

You may also need to change default value of DistvalHook. And since you are here, Could you also help to change the default of docstring ?

[irvingzhang0512]

Thanks for your clarification. It turns out that test failure is caused by Mock. Tests, Docs & default values are updated.

BTW, do we still need The interval of EvalHook should be divisible by that of CheckpointHook in the docstring? I thought it was designed for symlink

[codecov]

Codecov Report

Merging #600 (b48afa7) into master (2f7368c) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #600   +/-   ##
=======================================
  Coverage   84.31%   84.31%           
=======================================
  Files         124      124           
  Lines        8774     8774           
  Branches     1468     1468           
=======================================
  Hits         7398     7398           
  Misses       1019     1019           
  Partials      357      357           

Flag	Coverage Δ
unittests	84.30% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
mmaction/core/evaluation/eval_hooks.py	86.09% <ø> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f7368c...3db17ed. Read the comment docs.

[dreamerlin]

Thanks for your clarification. It turns out that test failure is caused by Mock. Tests, Docs & default values are updated.

BTW, do we still need The interval of EvalHook should be divisible by that of CheckpointHook in the docstring? I thought it was designed for symlink

Yeah, it is outdated, thanks for your correction!