Met4FoF / agentMET4FOF

Metrological Agent-based system (MET4FOF project)
Other
22 stars 8 forks source link

Incorrect docstring init_agent #307

Closed bangxiangyong closed 6 months ago

bangxiangyong commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.