Deprecate step argument in IntLogUniformDistribution.

[nzw0301]

Motivation

Since #1329 disables suggest_int to use step > 1 with log=True, we do not need to have step argument in IntLogUniformDistribution.

Description of the changes

This PR removes the step argument and refactors the sampling and tests codes.

[HideakiImamura]

Thanks for the good catch! LGTM!

[HideakiImamura]

Hi @nzw0301! I'm so sorry to comment after the approval. Could you take a look?

With careful consideration, I've noticed that the step argument of IntLogUniformDistribution has already appeared in v1.5.0. Therefore, removing the argument without any deprecation is unfriendly to users. Can we address this problem by not dropping the argument but deprecating it? Sorry for the confusion.

[nzw0301]

@HideakiImamura Sure! Please don't worry about it. So, do I just add a warning message to the constructor of IntLogUniformDistribution and revert step argument of IntLogUniformDistribution?

[HideakiImamura]

@nzw0301 Thanks for your swift reply! Yes, adding warning messages as the optuna._deprecated.deprecated decorator is a good approach. (The deprecation decorator cannot be applied to the argument. But the default deprecation note is given in optuna._deprecated.py.)

[nzw0301]

@HideakiImamura Thank you for your answer! I'm reading the codes and I think we cannot change the template of deprecated message defined in

optuna/optuna/_deprecated.py

Lines 14 to 20 in 0a2c099

	_DEPRECATION_NOTE_TEMPLATE = """
	.. warning::
	Deprecated in v{d_ver}. This feature will be removed in the future. The removal of this
	feature is currently scheduled for v{r_ver}, but this schedule is subject to change.
	See https://github.com/optuna/optuna/releases/tag/v{d_ver}.
	"""

So I think this deprecated decorator's message may confuse users for this issue as you mentioned. So I will add a warning message not using this decorator... How do you think?

[nzw0301]

In addition, even though I added the decorator to the IntLogUniformDistribution, no warning message appears because
warnings.warn with DeprecationWarning in

optuna/optuna/_deprecated.py

Lines 87 to 97 in 0a2c099

	@functools.wraps(func)
	def new_func(*args: Any, **kwargs: Any) -> Any:
	warnings.warn(
	"{} has been deprecated in v{}. "
	"This feature will be removed in v{}.".format(
	name if name is not None else func.__name__,
	deprecated_version,
	removed_version,
	),
	DeprecationWarning,
	)

is ignored by default unless it is triggered by code in __main__ (see ref). I suppose this behaviour is undesirable to notify users of the deprecation... (but my understanding may be wrong)

[HideakiImamura]

Thanks for your reply! Yes, this decorator can only be applied to the function or class, so we cannot apply the decorator to the argument. But, I think the contents of the deprecation massages should be same as that of the deprecation decorator. Do you think the ‘_DEFAULT_DEPRECATION_NOTE’ is not appropriate for the ‘step’ argument? (If we use it, the d_ver = deprecated version =2.0.0 and r_ver = removed version = 4.0.0.)

[nzw0301]

@HideakiImamura Yes, I do. _DEFAULT_DEPRECATION_NOTE sounds like IntLogUniformDistribution will be removed in the future...

[HideakiImamura]

Oh! I see. How about giving the deprecation note not to whole of the class but only to the ‘step’ argument explanation like https://github.com/optuna/optuna/blob/master/optuna/pruners/_hyperband.py#L117? Then, users can understand that the ‘step’ argument will only be removed.

[nzw0301]

I completely agree with you. I'm also thinking about your way 😎 .

[nzw0301]

Related to #1329, I change the codes such that step is replaced with 1 because suggest_int does not accept step != 1 when log=True. Internally, we do not need to hold step as an instance variable so it just shows the warning message when step != 1 in the constructor.

[HideakiImamura]

Thanks for your quick update! I have a minor comment.

[HideakiImamura]

Thanks for your swift action! LGTM again!

[toshihikoyanase]

Thank you for your PR. It basically looks good to me, but I have two small comments.

[toshihikoyanase]

Thank you for your update. It basically looks good to me, but let me add two small suggestions.

[toshihikoyanase]

Thank you for your update. LGTM!

@hvy I think you assigned yourself to this PR. Do you review this PR?

[hvy]

Sorry, I didn't mean to block. Thanks @nzw0301 for your contribution as always and let me merge this PR as it's already approved by two!

On a side note, the changes basically LGTM but I'm a bit worried that the deprecation is halfway in that the step attribute is actually removed and it'll break user-defined samplers. If you don't mind, I'd like to follow up this PR with putting it back with deprecation warnings raised.

[nzw0301]

Thank your kindful reviews as usual!

@hvy I appreciate your follow-up PR!