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

feat: FastSetUnification #7825

Open
wants to merge 29 commits into
base: master
Choose a base branch
from

Conversation

JonathanStarup
Copy link
Contributor

@JonathanStarup JonathanStarup commented Jun 3, 2024

Including fuzzer

@magnus-madsen
Copy link
Member

What's the plan for polish? I think it makes sense to do a thorough pass through the code before I look at it in detail.

@JonathanStarup
Copy link
Contributor Author

Yeah ill give it a full look over and ping you

@magnus-madsen
Copy link
Member

Yeah ill give it a full look over and ping you

Ok. Since this is a such a central piece of what we do, I have a high standard :)

It would be good to ensure all comments are up to date and that the code is as clean as possible.

Copy link
Member

@magnus-madsen magnus-madsen left a comment

Choose a reason for hiding this comment

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

I will add general comments before code-level comments:

  • Test cases do not seem to contain elements.
  • I am not sure I like SetEval it seems like an extra level of abstraction. At least it should not be written in OO-style.
  • Why does Term suddenly have both & and inter? How many ways do we know have to construct terms? :)
  • What is freeUnknowns?
  • toRawString ugh? Maybe some of this can be moved to a debugging class.
  • I still don't understand why Inter is defined the way it is with an option. It seems very asymmetric.

@JonathanStarup
Copy link
Contributor Author

  • Test cases do not seem to contain elements.
    • The fuzzed tests do. Do you want me to extract examples for std lib?
  • I am not sure I like SetEval it seems like an extra level of abstraction. At least it should not be written in OO-style.
    • I've changed it to func style.
    • I like that we don't have to know the universe. and its just 20 lines (excluding comments)
  • Why does Term suddenly have both & and inter? How many ways do we know have to construct terms? :)
    • Oversigt, its simplified now
  • What is freeUnknowns?
    • Constants and variables. Before we called flexify that converted csts to vars. instead evaluate now just views constants like that to begin with, saving a formula conversion
  • toRawString ugh? Maybe some of this can be moved to a debugging class.
    • Moved to the fuzzer. We can also delete it, it was just useful for debugging.
  • I still don't understand why Inter is defined the way it is with an option. It seems very asymmetric.
    • {e_12} ∩ {e_42} = empty so structurally we reflected that. union doesn't behave like that so it is asymmetric. I can also make it a zero/singleton set if you want.

@JonathanStarup
Copy link
Contributor Author

@magnus-madsen ive given it all a read-through and rewrite but it probably requires multiple, being so long.

@magnus-madsen
Copy link
Member

  • Test cases do not seem to contain elements.

    * The fuzzed tests do. Do you want me to extract examples for std lib?
    

Yes, definitely. This would be very useful.

  * Constants and variables. Before we called flexify that converted csts to vars. instead evaluate now just views constants like that to begin with, saving a formula conversion

Do we assume the domains are disjoint?

  * `{e_12} ∩ {e_42} = empty` so structurally we reflected that. union doesn't behave like that so it is asymmetric. I can also make it a zero/singleton set if you want.

But what about {throw, print} ∩ ef. Why is that not allowed to have a compact representation?

@JonathanStarup
Copy link
Contributor Author

Do we assume the domains are disjoint?

That was already assumed, noted in FastBoolUnification

Yes, definitely. This would be very useful.

It is less cool since we don't use minus yet, but I can find some examples

But what about {throw, print} ∩ ef. Why is that not allowed to have a compact representation?

we can/should do it for sure, but it adds extra logic in many of the functions. i think it be better not to make this PR larger

@magnus-madsen
Copy link
Member

It is less cool since we don't use minus yet, but I can find some examples

Regions should be their own effects. Then there is plenty to take from.

@magnus-madsen
Copy link
Member

we can/should do it for sure, but it adds extra logic in many of the functions. i think it be better not to make this PR larger

Since there is no dependency on this PR, I suggest we iterate as much as needed.

@JonathanStarup
Copy link
Contributor Author

Compact concrete sets:

  1. We could change the element part of an intersection to represent a union
    a. This means that the most compact element rep is an intersection and that {a} u {b} should simplify to Inter({a, b}, empty...)
    b. This allows simplifying fx ({a} u {b}) inter ({b} u {c}) since it becomes a nested intersection
  2. We could change Elem(Int) to ElemSet(Set[Int])
    a. both union and intersection will just have zero/one element set
    b. The most compact element rep is an element set

I think the second is the most comprehensible, do you have a comment @magnus-madsen?

@magnus-madsen
Copy link
Member

@JonathanStarup Can we do both?

@JonathanStarup
Copy link
Contributor Author

both? doesnt that just give inconsistent representation?

@JonathanStarup
Copy link
Contributor Author

Efficient really is the enemy of elegant

@magnus-madsen
Copy link
Member

We should definitely do (2) then we can fight over (1).

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.

None yet

2 participants