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

Incorrect docstring init_agent #307

Closed
bangxiangyong opened this issue Oct 10, 2023 · 3 comments · Fixed by #308
Closed

Incorrect docstring init_agent #307

bangxiangyong opened this issue Oct 10, 2023 · 3 comments · Fixed by #308
Assignees
Labels
cosmetics Something doesn't look right

Comments

@bangxiangyong
Copy link
Member

The docstring of init_agent in base_agents.py says Calls user-defined 'init_parameters()' upon finishing. However, this isn't the case as shown in lines 233-234 and 251-252 of network.py where it is the other way round: init_parameters is called first, followed by init_agent.

@BjoernLudwigPTB
Copy link
Member

BjoernLudwigPTB commented Oct 11, 2023

Thanks for bringing this up! Especially, no call of init_parameters() is part of init_agent() which is good, as the logic behind those methods does not overlap. Additionally both methods belong to different classes and even different of classes in different modules in different packages. There should be no coupling of the methods in the first place. I think, we should simply remove this statement from the docstring. Do you think, that would be appropriate @bangxiangyong ?

@bangxiangyong
Copy link
Member Author

Well, actually i was fiddling with the buffer_size parameter in init_agent() and wanted to instantiate it via init_parameters() instead. and was somewhat confused by the results which didn't tally: the init_agent() then overrides the buffer size despite what i did with the buffer size in init_parameters().

We could either remove it (which is an incorrect statement), or actually make it clearer, just in case there would be interest similar to what i went through. There could be other better ways perhaps

@BjoernLudwigPTB
Copy link
Member

BjoernLudwigPTB commented Oct 12, 2023

I'd say, the clarification should actually be made somewhere more visible. init_agent() is somewhat downstream and should not be thought about in general usage scenarios. The usual choice of buffer_size can be made during the initialization of the instance as is shown in the buffering tutorial. If one wanted to change the parameter, it should be done via calling init_parameters() where it then should be described and in fact taken care of as an optional parameter. In (maybe all) our examples and implementations I can't find a parameter buffer_size. Also, there actually already is an open issue #224 about unexpected behaviour of the parameter buffer_size. This looks like something we should put some thought in. But the removal of the misleading part of the docstring we agree upon, right? So this could already been done. I will prepare a PR later today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cosmetics Something doesn't look right
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants