-
Notifications
You must be signed in to change notification settings - Fork 43
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
Knowledge #210
base: dev
Are you sure you want to change the base?
Knowledge #210
Conversation
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.
Generally good concept and nice clean up of actions.
@@ -0,0 +1,3 @@ | |||
======================= | |||
Knowledge Examples | |||
======================= |
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.
is the inclusion of the example missing here?
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.
No, to have categories in the toc you need a page under which you can put other pages that's what this is for.
to be filled, otherwise a TypeError will be raised, see the example below for usage. | ||
|
||
.. code-block:: python | ||
# Example usage |
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.
shouldnt example usage be prefixed with
my_boject = ...
?
def generate_permutations(args: Iterable) -> List: | ||
""" | ||
Generates all possible permutations of the given arguments. This uses itertools.product to generate the | ||
permutations. |
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.
unnescessary information in the docstring
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.
write cartesian product or something that indicates what you are doing conceptually. If i want to know how you do it in detail i can read the code
@property | ||
def resolved(self) -> bool: | ||
""" | ||
Returns True if all nodes in the tree are resolved. This is done by iterating over the tree and checking if all |
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.
TMI
def simplify(self): | ||
""" | ||
Simplifies the tree structure by merging nodes of the same type. This is done by iterating over the tree and | ||
merging nodes of the same type. |
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.
TMI and duplicated?
class And(PropertyOperator): | ||
""" | ||
Class to represent a logical and operator in a tree structure. This class inherits from PropertyOperator and adds a | ||
method to evaluate the children as an and operator. |
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 am very unsure about the information thats given here. Like Inherits from is written in the class definition and the adds a method stuff down there too. Also it is required by the super class.
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.
Git shows this in a weird version. This file just got restructured didnt it?
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.
yep
src/pycram/knowledge/__init__.py
Outdated
# import os | ||
# import sys | ||
|
||
# path = os.path.dirname(os.path.abspath(__file__)) |
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.
delete if this is unwanted
|
||
from ..failures import KnowledgeNotAvailable, ReasoningError | ||
# This import is needed since the subclasses of KnowledgeSource need to be imported to be known at runtime | ||
from .knowledge_sources import * |
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.
if stuff like this is needed i recommend to import it in the init.py I got the same problems for the subclass json serializer but the best way to resolve the conflict for me was to import what i can in the init of a module and the rest (cyclic dependency stuff) has to be done by the user when he wants to use that class.
import glob | ||
|
||
modules = glob.glob(join(dirname(__file__), "*.py")) | ||
__all__ = [basename(f)[:-3] for f in modules if isfile(f) and not f.endswith('__init__.py')] |
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.
this looks fishy, please explain
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.
This imports all files in this directory to ensure that all knowledge sources are known.
from pycram.knowledge.knowledge_engine import KnowledgeEngine | ||
|
||
|
||
class TestProperty(Property): |
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.
docstring that these are testing dummies or write the name like PropertyMock. https://en.wikipedia.org/wiki/Mock_object
Description
Adds the new Knowledge and Reasoing engine