New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Keyword-only link argument in GradientMethod #3940
Conversation
fde37e4
to
8e9d41e
Compare
8e9d41e
to
8336560
Compare
# If the attribute is not defined as the class attribute of | ||
# `Hyperparameter`, it's assumed to be a hyperparameter. | ||
if not hasattr(Hyperparameter, name): | ||
if not (isinstance(value, (numpy.ndarray, cuda.ndarray)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose this change makes Hyperparameter less flexible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean you may want to set a hyperparameter which is not an array nor a scalar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
I've encountered the case I'd like to pass a string argument to an optimizer, which syncs parameters 'soft'
or 'hard'
. To be honest, my final code didn't pass the string, and I've not set a string value to a hyperparameter (because the optimizer was not an instance of GradientMethod
.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, we need to decide to which extent Hyperparameter
accepts as a value.
The most conservative option is to accept anything but Link
, but in this case we would need to add such rules in the future depending on how it is used. I don't think that's a good choice.
I think the minimum is scalars and ndarrays (as in the current code).
Maybe str
is reasonable, too.
Now that this argument has been removed (See #4141), I'm closing this PR. |
Improves #3488 by making
link
argument keyword-only.In #3488, this code does not work and dangerous because
model
is treated as a hyperparameter.In this PR, user must explicitly specify
link=
keyword.Also, type check on hyperparameter assignment is implemented, so the former code raises
TypeError
.I wondered which is better keyword,
link
ormodel
, but asOptimizer.setup()
method haslink
argument, I choselink
.