Skip to content
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

show error message #210

Merged
merged 4 commits into from
Jan 16, 2018
Merged

Conversation

corochann
Copy link
Member

@corochann corochann commented Dec 28, 2017

When we construct QFunction, and model parameter is not initialized (it happens when some Link is instantiated with in_size=None), copy_param will fail because the target_link's param is None.

This PR is to show user-friendly error message to let the user know what is the cause of this error.

I'd like to get comment that what kind of message is better.

I'm not sure the performance degrade by checking this type error.

@toslunar
Copy link
Member

Thanks. I agree with you about showing error message.

I'd like to see message that starts with "target_link has parameter {}", since target_params is a local name.

@corochann
Copy link
Member Author

corochann commented Dec 28, 2017

And should we add some sentence to how to fix this error like:

'target_link parameter {} is None. Maybe the model params are not initialized.\n Please forward dummy input before hand to determine parameter shape of the model.'.format(param_name)

or something?

@toslunar
Copy link
Member

It's a possible fix to forward dummy input. However, I'd rather not recommend it.
Instead default agents in ChainerRL compute the first action before doing the first training (in act_and_train).

@corochann
Copy link
Member Author

Updated text.

default agents in ChainerRL compute the first action before doing the first training (in act_and_train).

Yeah, if DQN agent uses parameter-initialized target_link it would be nice.

@corochann corochann changed the title [WIP] show error message show error message Jan 11, 2018
@toslunar
Copy link
Member

Sorry. You are right. DQN agent expects parameters of q_function to be initialized. Let's recommend forwarding dummy input on such uninitialized-parameter errors.

@corochann
Copy link
Member Author

corochann commented Jan 16, 2018

So you prefer below comment rather than current PR?

'target_link parameter {} is None. Maybe the model params are not initialized.\n Please forward dummy input before hand to determine parameter shape of the model.'.format(param_name)

@toslunar
Copy link
Member

Yes, I prefer the longer comment.

Some minor fixes:

  • \n Please -> \nPlease
  • Please -> Please try to (?)
  • before hand -> beforehand

@toslunar
Copy link
Member

Could you add testcases (in tests/misc_tests/test_copy_param.py) if you could spare the time?

Copy link
Member

@toslunar toslunar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@toslunar toslunar merged commit 99da574 into chainer:master Jan 16, 2018
@muupan muupan added this to the v0.4 milestone Jul 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants