fvutils / pyvsc

Python packages providing a library for Verification Stimulus and Coverage
https://fvutils.github.io/pyvsc
Apache License 2.0
113 stars 26 forks source link

Skip disabled constraints when visiting model; Fixes #173 #174

Closed alwilson closed 1 year ago

alwilson commented 1 year ago

Traced the bounds processing to ModelVisitor and added a constraint enabled check.

Added a unittest that passes with this commit.

mballance commented 1 year ago

Hi @alwilson, thanks for taking the time to dig into this issue! If I understand correctly from issue #173, it seems that the core problem is that the bounds visitor isn't ignoring disabled constraints. If that is the case, I would recommend overriding the visit_constraint_block method from ModelVisitor (see below) in the bounds visitor class and only visit the base implementation if the constraint is enabled. https://github.com/fvutils/pyvsc/blob/743b6d3c3376dfa15feab91bfe454f5ee5a53eac/src/vsc/model/model_visitor.py#L106-L107

It should look something like:

class VariableBoundVisitor(ModelVisitor):
    # ...

    def visit_constraint_block(self, c : ConstraintBlockModel):
        if c.enabled:
            super().visit_constraint_block(c)
    # ...

This will allow other users of the ModelVisitor class to receive notifications of disabled and enabled constraint blocks, while blocking the bounds visitor from taking disabled constraints into account.

alwilson commented 1 year ago

Ah, reread your comment after going back. I need to play with git some more too. Haven't done much rebase/squash stuff from the command line before.

mballance commented 1 year ago

@alwilson, don't worry about squashing commits or rebasing -- this is a nice small change. If it were me, I'd just copy/paste the visit_constraint_block method over to the bounds visitor class, revert the changes in the model_visitor class, and call it it a day. Your call, though :-)

alwilson commented 1 year ago

Yeah, it does feel like a hole in my git knowledge... but I think even github will let you squash merge as well if you wanted. I recall seeing options for it before at least. Thanks for the feedback!

mballance commented 1 year ago

Many thanks, @alwilson!