Skip to content
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

Clock reduction fix #151

Merged
merged 8 commits into from
Mar 30, 2023
Merged

Clock reduction fix #151

merged 8 commits into from
Mar 30, 2023

Conversation

t-lohse
Copy link
Contributor

@t-lohse t-lohse commented Mar 16, 2023

Before, the clock reduction could not handle queries such as "refinement: A <= A" -i samples/json/AG where A has unused clocks within it. It would panic, trying to remove clocks on the left side of the expression, tat where actually found on the right side. This PR provides a fix to that.

Beyond that, when I created the tests, some other tests were panicking. This was because they expected an Ok from create_executable_query with a failure withing, but clock reduction compiles the system and returns the failure in Err if one is found. The reason for this is because these tests used ConsistencyExecutor, which has teh only struct implementing ExecutableQuery that took a SystemRecipe and compiled the system itself instead of taking a TransitionSystemPtr.
Therefore, I have refactored that, and a helper-function (json_run_query) in the tests so we can return the failure when performing clock reduction, instead of when executing the query. This involved a good portion of refactoring (hence the size of the PR), however it is mostly adapting the usage of said helper function

@t-lohse t-lohse marked this pull request as draft March 17, 2023 12:02
@t-lohse t-lohse marked this pull request as ready for review March 19, 2023 13:24
@t-lohse t-lohse requested a review from seblund March 19, 2023 13:24
Copy link
Member

@seblund seblund left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall!

I left a few comments, but they are quick fixes if you agree.

Nice catch on the change for the ConsistencyExecutor, it is now instantiated exactly like the DeterminismExecutor, which is great. When we eventually add the specification and implementation executors we should probably introduce a helper function as they will all four will share the exact same instantiation code in extract_system_rep.rs.

src/ModelObjects/component.rs Show resolved Hide resolved
src/tests/refinement/Helper.rs Outdated Show resolved Hide resolved
src/tests/system_recipe/conjunction.rs Outdated Show resolved Hide resolved
@t-lohse t-lohse requested a review from seblund March 30, 2023 08:12
Copy link
Member

@seblund seblund left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@seblund seblund left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still looks good :)

@t-lohse t-lohse merged commit ba0583e into new_results_refactor Mar 30, 2023
@t-lohse t-lohse deleted the clock-reduction-fix branch March 30, 2023 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants