René's URL Explorer Experiment

Title: Update model.py by ren-oz · Pull Request #13 · CarletonCognitiveModelingLab/python_actr · GitHub

Open Graph Title: Update model.py by ren-oz · Pull Request #13 · CarletonCognitiveModelingLab/python_actr

X Title: Update model.py by ren-oz · Pull Request #13 · CarletonCognitiveModelingLab/python_actr

Description: Cleaned up (deleted) old comments, made code PEP-8 compliant. Updated Python 2 string formatting to Python 3 f-strings. Fixed bug related to initializing children -- does not work as expected. The issue can be recreated with the following code: from python_actr import ACTR, Model env = Model() env.a1 = ACTR() print(env.get_children()) # List is empty print(env._children) # Raises AttributeError: "'Model' object has no attribute '_children'." env.a2 = ACTR() print(env.get_children()) # List has only one value print(env._children) # No exception raised, but only a2 is in children! This is resolved by moving self.__dict__[key] = value to the end of the __setattr__ method in class Model. The problem occurs because (if I am correct) whenever a class attribute of type Model is set for the first time in a parent Model instance (e.g., an environment), the parent class instance is forced into a conversion method (self._ensure_converted). During this conversion process, all Model instances in the parent namespace are also converted via: for k, v in list(self.__dict__.items()): if k[0] != '_' and k != 'parent' and isinstance(v, Model): if not v.__converted: v.__convert(parent=self) This leads to the child instance having its parent attribute set, and thus the parent _children attribute does not get set due to if getattr(value, 'parent', None) is not None: pass Interestingly, commenting out the for k, v in list(self.__dict__.items()): [...] block doesn't seem to break anything (hasn't been extensively tested though) and also resolves the issue as the children conversion happens later on in __setaatr__ along with an explicit setting of the parent _children attribute. But this is a larger block of code and must have been put in for some reason, so I'm hesitant to commit its deletion in this pull request. Future changes to consider: Ultimately, the methods in this file have low cohesion and are responsible for a lot of unrelated work. Refactoring model.py to delegate single responsibilities to other methods is something to strongly consider as it would help make maintenance easier in the event of future bugs. It should also help in developing new subclasses with less pain and ideally favour extensibility. That said, this must be done with care as much is built on top of this module so the changes could snowball. I am happy to assist in this process with pull requests if the team deems it worthwhile.

Open Graph Description: Cleaned up (deleted) old comments, made code PEP-8 compliant. Updated Python 2 string formatting to Python 3 f-strings. Fixed bug related to initializing children -- does not work as expected. The ...

X Description: Cleaned up (deleted) old comments, made code PEP-8 compliant. Updated Python 2 string formatting to Python 3 f-strings. Fixed bug related to initializing children -- does not work as expected. The ...

Opengraph URL: https://github.com/CarletonCognitiveModelingLab/python_actr/pull/13

X: @github

direct link

Domain: patch-diff.githubusercontent.com

Links:

Viewport: width=device-width