Fix lex issue in #456 #477
Conversation
|
All the tests should pass on this since the only failing case was the unexpected allDiff output in cyclic_graph. |
|
@ozgurakgun I suggest we merge this. Azure seems to be happy but it is not reporting completion through to the github hooks for some reason. |
|
I can have a look later today.
… On 4 Mar 2020, at 09:16, Fraser Dunlop ***@***.***> wrote:
@ozgurakgun I suggest we merge this. Azure seems to be happy but it is not reporting completion through to the github hooks for some reason.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
| @@ -0,0 +1,3 @@ | |||
| find A : set of int(1..10) | |||
| find B : set of int(1..10) | |||
| such that A .< B | |||
There was a problem hiding this comment.
Should we be testing for this in the input? I am sure we don't support .< between two different representations. You get away with it here thanks to compact.
Just do find s : set (size 2) of set of int(1..10) which would create .< constraints internally then refine them?
There was a problem hiding this comment.
Looking at the rest of the files now I see there is a bunch of other .< constraints in the input.
Maybe we can group them in a tests/custom/dotless directory? I think these will eventually go and be replaced by just <.
There was a problem hiding this comment.
I thought the plan was to get .< working in the way we want < to be? Or should it only be supporting the core tree language?
There are lots of tests with .< I think. They will of course be changed to < when we upgrade its power.
There was a problem hiding this comment.
We want to leave .< alone. It works by treating all of the variables of the representation as a block and putting lex between them. The two operands must have the same domain and must be represented in the same way.
We also want to leave ~< alone as well. This is a correct way of ordering anything independent of representation or domain. They must have the same type of course.
We want to implement < for all types using symmetryOrder etc. The hope is this new family of comparators will be as efficient as .< and as generic as ~<.
There was a problem hiding this comment.
Okay. Currently I match on .< and flatten both sides. So I should instead do that with <?
There was a problem hiding this comment.
Okay, I just want to conform to the spec. What do you mean by touching .< at all?
Do you want to revert to rule_DotLtLeq before it called symmetryOrdering and introduce a rule_LtLeq that does what rule_DotLtLeq currently does?
There was a problem hiding this comment.
I mean, AFAIK, before this PR the dot-less handling is correct. And we don't want to change it. We don't necessarily want to test it in this way either since it is generated as part of almost all tests and works correctly anyway.
Am I missing something?
And yes, the new implementation that will work on every operand should just work on <.
There was a problem hiding this comment.
Okay, so if I remove the test on sets which doesn't make sense for .< and clean up the comments in the expected files we should be good to merge?
I'll work on the < implementation in a separate branch from this.
There was a problem hiding this comment.
Sounds good to me! I am at work today so should be able to stay in the loop as you commit more stuff to this pr.
| branching on [A_Occurrence, B_Occurrence] | ||
| such that [-toInt(A_Occurrence[q3]) | q3 : int(1..10)] <lex [-toInt(B_Occurrence[q4]) | q4 : int(1..10)] | ||
|
|
||
| $ Conjure's |
There was a problem hiding this comment.
I really don't like committing the comment lines as part of test results. Use head -n7 to filter them out?
| flatten([flatten([[-toInt(A_1[q3]); int(1)], [-toInt(A_2[q3]); int(1)]; int(1..2)]) | q3 : int(0..1)]) <lex | ||
| flatten([flatten([[-toInt(B_1[q4]); int(1)], [-toInt(B_2[q4]); int(1)]; int(1..2)]) | q4 : int(0..1)]) | ||
|
|
||
| $ Conjure's |
There was a problem hiding this comment.
Same here with the comment lines.
There was a problem hiding this comment.
I think I will have to go through the lot and apply a filter. Not a problem though.
There was a problem hiding this comment.
If you want a generic filter see this.
|
Hi @fraser-dunlop - I am keen to merge this but I see the .< tests (and the comments in them) are still in the PR. Do you want to keep them? What's your plan? I have a bunch of local changes that I'd like to push (shouldn't conflict with yours) but wanted to check with you first. |
We can go ahead and merge I think. I used this to remove the comments Which tests are you referring to? If it is a case of .< in the tests when it should be < then I think we can merge and deal with that in another PR. |
|
I meant this kind of thing https://github.com/conjure-cp/conjure/pull/477/files#diff-b87fb0fdc2e851ec258b7f285b7aaace |
|
Sorry, must have missed those. They are all fine now I think... |
I have added the failing examples from #456 as regression tests and fixed the underlying bug. The fix was relaxing the filter on rule_Flatten_Lex so that it no longer was activated for types that don't need flattening.