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.

Sign up for GitHub

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

Jump to bottom

Make Reduction new_top, and Model constraints a vector #421

Closed
YehorBoiar opened this issue Nov 7, 2024 · 1 comment
Closed

Make Reduction new_top, and Model constraints a vector #421

YehorBoiar opened this issue Nov 7, 2024 · 1 comment
Assignees
@YehorBoiar
Labels
area::conjure-oxide/rule-engine Related to the rule engine and the expression rewriting logic. area::conjure-oxide Related to conjure_oxide. kind::question kind::refactor Improvements to existing code (style, performance, clarity, ...) planned

Comments

@YehorBoiar
Copy link
Contributor

We would like to make new_top an attribute in Reduction and constraints in Model a vector. I attempted to implement this in PR #370 but was unsuccessful.

@ozgurakgun, could you clarify the benefits of using vectors for these attributes? I understand that it looks cleaner and might use less space, as an empty vector is smaller than Expression::And(Metadata::new(), Vec::new()). However, are there other reasons I should consider while implementing this change?
@YehorBoiar YehorBoiar self-assigned this Nov 7, 2024
@YehorBoiar YehorBoiar added area::conjure-oxide Related to conjure_oxide. kind::refactor Improvements to existing code (style, performance, clarity, ...) area::conjure-oxide/rule-engine Related to the rule engine and the expression rewriting logic. kind::question labels Nov 7, 2024
@YehorBoiar YehorBoiar changed the title Making new_top, and Model.constraints a vector Make Reduction new_top, and Model constraints a vector Nov 7, 2024
@ozgurakgun
Copy link
Contributor

I am not sure if it uses less space etc. For me the major benefit is being close to the input model, which in turn means if we pretty print the model it would look closer to what the user wrote.

For reference, this is how model is defined in Conjure: https://github.com/conjure-cp/conjure/blob/4bae499bcc7ec93a69ea0f2ea46078544ddb097c/src/Conjure/Language/Definition.hs#L76

Also see Statement and Expression in that file.

I think we should reproduce this, in addition we will keep context (including symbol table) as well.

@YehorBoiar YehorBoiar mentioned this issue Nov 11, 2024
Merged
This was referenced Nov 28, 2024
Closed
Open
YehorBoiar added a commit that referenced this issue Dec 7, 2024
@YehorBoiar
Refactor test logic
17d3fe7 
In PR #421, tests were unintentionally passing due to expected files being overwritten regardless of whether the generated solutions matched Conjure’s solutions. This commit refactors the integration_test_inner function and related file I/O utilities so that:
	- When ACCEPT=false, tests still compare generated outputs (parse, rewrite, solutions, and traces) against the expected results as before.
	- When ACCEPT=true, tests now first verify that the generated solutions match the Conjure solutions. Only if they match are the expected files updated. If they don’t match, the test fails without updating the expected files.
niklasdewally pushed a commit that referenced this issue Dec 9, 2024
@YehorBoiar @niklasdewally
Refactor test logic
a1cef35 
In PR #421, tests were unintentionally passing due to expected files being overwritten regardless of whether the generated solutions matched Conjure’s solutions. This commit refactors the integration_test_inner function and related file I/O utilities so that:
	- When ACCEPT=false, tests still compare generated outputs (parse, rewrite, solutions, and traces) against the expected results as before.
	- When ACCEPT=true, tests now first verify that the generated solutions match the Conjure solutions. Only if they match are the expected files updated. If they don’t match, the test fails without updating the expected files.
niklasdewally pushed a commit that referenced this issue Dec 9, 2024
@YehorBoiar @niklasdewally
Refactor test logic
58cee04 
In PR #421, tests were unintentionally passing due to expected files
being overwritten regardless of whether the generated solutions matched
Conjure’s solutions.

This commit refactors the integration_test_inner function and related
file I/O utilities so that:

* When ACCEPT=false, tests still compare generated outputs (parse,
  rewrite, solutions, and traces) against the expected results as
  before.

* When ACCEPT=true, tests now first verify that the generated solutions
  match the Conjure solutions. Only if they match are the expected files
  updated. If they don’t match, the test fails without updating the
  expected files.
@YehorBoiar YehorBoiar mentioned this issue Dec 10, 2024
Closed
YehorBoiar added a commit that referenced this issue Feb 5, 2025
@YehorBoiar
Refactor test logic. Break general stages into substages
37057e8 
In PR #421, tests were unintentionally passing due to expected files
being overwritten regardless of whether the generated solutions matched
Conjure’s solutions.

This commit refactors the integration_test_inner function and related
file I/O utilities so that:

* When ACCEPT=false, tests still compare generated outputs (parse,
  rewrite, solutions, and traces) against the expected results as
  before.

* When ACCEPT=true, tests now first verify that the generated solutions
  match the Conjure solutions. Only if they match are the expected files
  updated. If they don’t match, the test fails without updating the
  expected files.

Stages desciption:
	1a: Parse (Convert the Essence model into an internal format.)
	1b: Check against other parser(s) (Compare output from different parsing methods (e.g., native parser vs JSON parser).)
	2a: Rewrite (Apply transformations to the parsed model (e.g., simplifying expressions, normalizing).)
	2b: Check model properties (extra_asserts) (Verify additional model properties (e.g., ensure vector operators are evaluated).)
	3a: Solve (Run the model through the solver to find solutions.)
	3b: Check solutions against Conjure (Compare solutions with expected output from the Conjure solver.)
	4a: Check that the generated rules match expected traces
YehorBoiar added a commit that referenced this issue Feb 6, 2025
@YehorBoiar
Refactor test logic. Break general stages into substages
aea8b16 
In PR #421, tests were unintentionally passing due to expected files
being overwritten regardless of whether the generated solutions matched
Conjure’s solutions.

This commit refactors the integration_test_inner function and related
file I/O utilities so that:

* When ACCEPT=false, tests still compare generated outputs (parse,
  rewrite, solutions, and traces) against the expected results as
  before.

* When ACCEPT=true, tests now first verify that the generated solutions
  match the Conjure solutions. Only if they match are the expected files
  updated. If they don’t match, the test fails without updating the
  expected files.

Stages desciption:
	1a: Parse (Convert the Essence model into an internal format.)
	1b: Check against other parser(s) (Compare output from different parsing methods (e.g., native parser vs JSON parser).)
	2a: Rewrite (Apply transformations to the parsed model (e.g., simplifying expressions, normalizing).)
	2b: Check model properties (extra_asserts) (Verify additional model properties (e.g., ensure vector operators are evaluated).)
	3a: Solve (Run the model through the solver to find solutions.)
	3b: Check solutions against Conjure (Compare solutions with expected output from the Conjure solver.)
	4a: Check that the generated rules match expected traces
YehorBoiar added a commit that referenced this issue Feb 7, 2025
@YehorBoiar
Refactor test logic. Break general stages into substages
74fbd4b 
In PR #421, tests were unintentionally passing due to expected files
being overwritten regardless of whether the generated solutions matched
Conjure’s solutions.

This commit refactors the integration_test_inner function and related
file I/O utilities so that:

* When ACCEPT=false, tests still compare generated outputs (parse,
  rewrite, solutions, and traces) against the expected results as
  before.

* When ACCEPT=true, tests now first verify that the generated solutions
  match the Conjure solutions. Only if they match are the expected files
  updated. If they don’t match, the test fails without updating the
  expected files.

Stages desciption:
	1a: Parse (Convert the Essence model into an internal format.)
	1b: Check against other parser(s) (Compare output from different parsing methods (e.g., native parser vs JSON parser).)
	2a: Rewrite (Apply transformations to the parsed model (e.g., simplifying expressions, normalizing).)
	2b: Check model properties (extra_asserts) (Verify additional model properties (e.g., ensure vector operators are evaluated).)
	3a: Solve (Run the model through the solver to find solutions.)
	3b: Check solutions against Conjure (Compare solutions with expected output from the Conjure solver.)
	4a: Check that the generated rules match expected traces
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@YehorBoiar YehorBoiar

Labels
area::conjure-oxide/rule-engine Related to the rule engine and the expression rewriting logic. area::conjure-oxide Related to conjure_oxide. kind::question kind::refactor Improvements to existing code (style, performance, clarity, ...) planned
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@ozgurakgun @niklasdewally @YehorBoiar