nickovic / rtamt

Specification-based real-time monitoring library
BSD 3-Clause "New" or "Revised" License
50 stars 20 forks source link

AST visitor refactoring #88

Closed TomyYamy closed 2 years ago

TomyYamy commented 2 years ago

I'm looking commit: c2b6dbdc4edea3946c7cd4c7c5793a96fe577221 in ast-refactoring.

I feel current AbstractVisitor is not refereed from the other class. rtamt/rtamt/ast/visitor/abstract_visitor.py for instance STLASTVisitor does not refer it rtamt/rtamt/ast/visitor/stl/ASTVisitor.py

TomyYamy commented 2 years ago

My intention of visitor is in commit: 095289e6885cd59f7688cfcc268ae9b8626623d7 in TLabstractClassTry3. please run,

cd rtamt/examples/offline_monitors python2 test_printName.py

Sorry It works only python2. but it is just path issue.

Actually it is not ideal code. Please see /rtamt/rtamt/ast/parser_visitor/stl/rtamtASTvisitor.py

class STLVisitor:
__metaclass__ = ABCMeta

def visit(self, node, args):
    out = None

    if isinstance(node, Predicate):
        self.visit(node.children[0], args)
        self.visit(node.children[1], args)
        out = self.visitPredicate(node, args)

It visits children manually.

TomyYamy commented 2 years ago

On the other hand, commit: 723207008c3bceb548dcdc183718df1334429a2e in semantics2 has more ideal code.

Please see, rtamt/rtamt/ast/parser_visitor/abstract_visitor.py

from abc import ABCMeta, abstractmethod
class AbstractVisitor(object):
__metaclass__ = ABCMeta
def visitChildren(self, node, *args, **kwargs):
    children_output = list()
    for nodeChild in node.children:
        output = self.visit(nodeChild, *args, **kwargs)
        children_output.append(output)
    return children_output

def visit(self, node, *args, **kwargs):
    return self.visitChildren(node, *args, **kwargs)

It does not call visit one by one. just call visitChildren.

But to implement that we need to refactor all other evaluators like this, /mnt/data/repos/rtamt/rtamt/semantics/discrete_time/offline/stl/eval_visitor.py

class STLOfflineDiscreteTimePythonEvalVisitor(STLrtamtASTvisitor):
    def __init__(self, spec):
        super(STLOfflineDiscreteTimePythonEvalVisitor, self).__init__(STLrtamtASTvisitor, spec)

    def visitPredicate(self, node, *args, **kwargs):
        children_out = self.visitChildren(node, *args, **kwargs)
        in_sample_1 = children_out[0]
        in_sample_2 = children_out[1]

so, all node input and output needs to change to follow this. Just the refactoring is many, but the change itself is simple when I tried it in another branch.

TomyYamy commented 2 years ago

Maybe offline eval works with the visitor. I think down side is how to manage online visitor that need both of back and forward propagation.

TomyYamy commented 2 years ago

However, I think I think we can follow some how ANTRL4 auto generated PaserVisitor rtamt/rtamt/parser/stl/StlParserVisitor.py

class StlParserVisitor(ParseTreeVisitor):

# Visit a parse tree produced by StlParser#interval.
def visitInterval(self, ctx):
    return self.visitChildren(ctx)

it has only visitChildren as a default method. We don't need to think how many node under this node.

TomyYamy commented 2 years ago

I think I can implement it on another branch if this down side is OK as a prototyping

TomyYamy commented 2 years ago

Our discussion was,

  1. let's keep current STLSpesificationParser because it must follow ANTRL 100% Maybe we need documentation...
    1. Problem is RTAMT side node visitor handling. How much simply if while referring implementation of ANTRLVisitor
TomyYamy commented 2 years ago

Let's discuss AST visitor only in here since we agreed AST parser follows ANTRL 100%. We may start the discussion after fix parse() location #93 .

TomyYamy commented 2 years ago

I'm thinking this topic because of #93 is closed to closing. I'm thinking some how merging branch; TLabstractClassTry3 and semantics2. My points are,

  1. Abstract visitor can visit only abstract nodes;BainaryNode, UniaryNode, information.
  2. Each operation of visitor can see next children explicitly rather than variadic arguments

My question is evaluation side, even-though here is AST layer, we can not avoid the semantic layer implementation. because here is connection. Current evaluation is implemented in different python codes separately, like this

def update(self, *args, **kargs):
    out = []
    input_list = args[0]

    next = float("nan")

    for i, in_sample in reversed(list(enumerate(input_list))):
        out_time = in_sample[0]
        out_value = min(in_sample[1], self.next)
        self.next = out_value
        if out_value == next and i < len(input_list) - 2:
            out.pop(0)
        out.insert(0, [out_time, out_value])
        next = out_value

    return out 

@nickovic I feel it is pain for programmer. Do you think we can implement the evaluator in plane in extended AST visitor class?

TomyYamy commented 2 years ago

I'm proposing AST visitor in commit; 4429f9b97f119a7742bf7f454a0c367584841704 Branch; ast-visitor-refactoring The main part would be /rtamt/rtamt/ast/visitor/abstract_ast_visitor.py

    from abc import ABCMeta, abstractmethod

    from rtamt.node.unary_node import UnaryNode
    from rtamt.node.binary_node import BinaryNode
    from rtamt.node.leaf_node import LeafNode

    from rtamt.exception.exception import AstVisitorException

    class AbstractAstVisitor(object):
        __metaclass__ = ABCMeta

        def visitAbstractAstChildren(self, node, *args, **kwargs):
            if isinstance(node, UnaryNode):
                sample_from_child = self.visitAbstractAstChildren(node.children[0])
                sample_return = self.visitSpecific(node, sample_from_child, *args, **kwargs)
                sample_return = None
                return sample_return
            elif isinstance(node, BinaryNode):
                sample_from_child_left = self.visitAbstractAstChildren(node.children[0])
                sample_from_child_right = self.visitAbstractAstChildren(node.children[1])
                sample_return = self.visitSpecific(node, sample_from_child_left, sample_from_child_right, *args, **kwargs)
                sample_return = None
                return sample_return
            if isinstance(node, LeafNode):
                sample_return = self.visitSpecific(node, *args, **kwargs)
                sample_return = None
                return sample_return
            else:
                raise AstVisitorException('{} is not RTAMT AST node'.format(node.__class__.__name__))

        def visit(self, ast, *args, **kwargs):
            self.ast = ast
            sample_return = self.visitAbstractAstChildren(self.ast.ast, *args, **kwargs)
            return sample_return, self.ast

        @abstractmethod
        def visitSpecific(self, node, *args, **kwargs):
            sample_return = None
            return sample_return

This code allows visit the AST with only abstract nodes' information.

I have done LTL and STL. Please see print name visitor, /rtamt/rtamt/ast/visitor/stl/print_name_visitor.py test case as well. /rtamt/tests/python/ast/test_ast_parseing.py

TomyYamy commented 2 years ago

@nickovic Do you think it can support your pastifier? I'm thinking try offline evaluation before next meeting. I think still we need to think more to support online with this visitor...

TomyYamy commented 2 years ago

I think from user, the node gets UnaryNode: XXXvisit(self, node, sample, *args, kwargs) BinaryNode: XXXvisit(self, node, left_sample, right_sample, *args, *kwargs) LeafNode: XXXvisit(self, node, args, kwargs) with back-forward fashion.

Problem is all pastifier, offline, online can align it or not. I think at least offline is OK and I'm attempting it.

TomyYamy commented 2 years ago

We all agree support,

TomyYamy commented 2 years ago

I proposed new one. Please see https://github.com/nickovic/rtamt/blob/f36c5b928b15e19e07faf6c9fbc487a3c3cad257/rtamt/ast/visitor/abstract_ast_visitor.py#L1-L75

I'm not 100% confident about it, because user can call specific visitor with different visit fashion (Manual, BottomUp, TopDown). I tried to make them to different visitors. but in this case it difficult to commonly use visitSpesific() that mapping visit for each specific TL. So finally I make them as just method of AbstractAstVisitor class.

TomyYamy commented 2 years ago

Now we agreed.

Remaining Topic. We did not see good use case of abstract nodes; UnaryNode, BinaryNode, and LeafNode

TomyYamy commented 2 years ago

We agreed somehow we will merge 88-tom-attempt to ast-refactoring

TomyYamy commented 2 years ago

@nickovic I merged my branch to ast-refactoring. Please implement your topic.

Only the biggest change is add vistAst(). since we agreed ast structure in #97, ast is not anymore, it contains dictionaries. so, previous ast is ast.ast. Hence I give vistAst() as a initial point of ast visitor. https://github.com/nickovic/rtamt/blob/b30b9207699f724250db366b50a69cfab8a37af8/rtamt/ast/visitor/abstract_ast_visitor.py#L29-L30

TomyYamy commented 2 years ago

@nickovic Now I come up with. Maybe we can use abstract visitor for reset() in online monitoring

TomyYamy commented 2 years ago

Sorry I had small revision for visitAst(). the class need to contain ast itself. https://github.com/nickovic/rtamt/blob/14ed8191248227ab93ec3b99d4573b95014ae079/rtamt/ast/visitor/abstract_ast_visitor.py#L29-L31 Perhaps we need to return ast itself too...

TomyYamy commented 2 years ago

@nickovic I'm thinking visitor args are

def visit(self, node, *args, **kwargs):

https://github.com/nickovic/rtamt/blob/178680228d872b9d1a4aa870cd52ae4293700603/rtamt/ast/visitor/stl/ast_visitor.py#L14-L30

But sometimes you use

 def visit(self, node, args):

Do we need to discuss it?

TomyYamy commented 2 years ago

@nickovic I need it for spec layer refactoring. You said you had finished it. But I did not see the branch. Just could you please tell me the branch and code location?

TomyYamy commented 2 years ago

We are done.