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

Predicate support #656

Merged
merged 19 commits into from
Sep 28, 2024
Merged

Predicate support #656

merged 19 commits into from
Sep 28, 2024

Conversation

tobiasgrosser
Copy link
Collaborator

No description provided.

Copy link

Alive Statistics: 76 / 93 (17 failed)

Copy link

Alive Statistics: 76 / 93 (17 failed)

Copy link

Alive Statistics: 76 / 93 (17 failed)

@bollu bollu marked this pull request as ready for review September 28, 2024 21:34
@bollu
Copy link
Collaborator

bollu commented Sep 28, 2024

I suggest we merge this in, and develop more theory upstream. Thanks a bunch @AtticusKuhn for filling in the proof! I cleaned up the PR, closed another sorry, and made the project build.

@bollu
Copy link
Collaborator

bollu commented Sep 28, 2024

@tobiasgrosser Do we want to keep the old implementation compiled? We are currently adding new fields, so the problem is that the old implementation does not know about repeatBit. Updating the old implementation to be in sync seems like a major pain point to me.

@tobiasgrosser
Copy link
Collaborator Author

I suggest we merge this in, and develop more theory upstream. Thanks a bunch @AtticusKuhn for filling in the proof! I cleaned up the PR, closed another sorry, and made the project build.

I think this is a great idea.

@tobiasgrosser
Copy link
Collaborator Author

@tobiasgrosser Do we want to keep the old implementation compiled? We are currently adding new fields, so the problem is that the old implementation does not know about repeatBit. Updating the old implementation to be in sync seems like a major pain point to me.

I need more context to understand the issue. What is the problem with the old implementation? In fact, do we not just build on-top of the old implementation?

@bollu
Copy link
Collaborator

bollu commented Sep 28, 2024

@tobiasgrosser we have two implementations, SSA/Experiments/Bits/Decide.lean and SSA/Experimental/Bits/Fast/Decide.lean. Both of these used to share the same definitions from SSA/Experiments/Bits/Defs.lean. But now, we added a field to Defs (repeatBit), which causes the old decision procedure in Bits/Decide.lean to fail.

Our automata / bitstream definitions all live in Bits/Fast, and we're building on top of that, which is also what the bv_automata tactic is based on.

Copy link

Alive Statistics: 76 / 93 (17 failed)

@bollu
Copy link
Collaborator

bollu commented Sep 28, 2024

@tobiasgrosser my suggestion is to delete Bits/{Defs.lean, Decide.lean}, and continue work on Bits/Fast/Decide.lean.

@tobiasgrosser
Copy link
Collaborator Author

tobiasgrosser commented Sep 28, 2024

@tobiasgrosser we have two implementations, SSA/Experiments/Bits/Decide.lean and SSA/Experimental/Bits/Fast/Decide.lean. Both of these used to share the same definitions from SSA/Experiments/Bits/Defs.lean. But now, we added a field to Defs (repeatBit), which causes the old decision procedure in Bits/Decide.lean to fail.

Our automata / bitstream definitions all live in Bits/Fast, and we're building on top of that, which is also what the bv_automata tactic is based on.

Can you split them? E.g., just copy the definitions into Fast, and develop them independently for now?

This allows us to maintain the previous code while not being held back by the old one.

@bollu
Copy link
Collaborator

bollu commented Sep 28, 2024

@tobiasgrosser that's exactly what I did in the current PR.

@tobiasgrosser
Copy link
Collaborator Author

Cool. Then lets merge.

@tobiasgrosser
Copy link
Collaborator Author

If they are decoupled, there is no need to delete them, no? It might still be useful to have them in a repo, as updating them to the latest lean won't have a cost. However, I don't feel super strong here. Happy to get see them dropped, too.

@bollu
Copy link
Collaborator

bollu commented Sep 28, 2024

Let's merge without dropping then, in case it turns out to be useful to reference the old impl.

@bollu
Copy link
Collaborator

bollu commented Sep 28, 2024

@tobiasgrosser hit merge if you're happy.

@tobiasgrosser tobiasgrosser added this pull request to the merge queue Sep 28, 2024
Merged via the queue into main with commit aac5225 Sep 28, 2024
2 checks passed
| i + 1 => by simp [compose_first i ((f a).1) f]

/--
Coinduction principle for `corec`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would call this bisimulation, rather than coinduction

Comment on lines +318 to +323
/-- `repeatBit xs` will repeat the first bit of `xs` which is `true`.
That is, it will be all-zeros iff `xs` is all-zeroes,
otherwise, there's some number `k` so that after dropping the `k` least
significant bits, `repeatBit xs` is all-ones. -/
def repeatBit (xs : BitStream) : BitStream :=
corec (b := (false, xs)) fun (carry, xs) =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought we wanted to change this name? I like foldOr more than repeatBit

Comment on lines +807 to +810
def Predicate.denote : Predicate α → Prop
| eq t1 t2 => t1.eval = t2.eval
| and p q => p.denote ∧ q.denote
| or p q => p.denote ∨ q.denote
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not quite right! We need to be taking in the arguments at the top-level, and pass those around.
Currently, we're denoting eq t u as equality of the functions that are denoted by t and u, meaning that we're universally quantifying each con-/disjunct separately.

def Predicate.denote (xs : α → BitStream) : Predicate α → Prop
| eq t1 t2 => t1.eval xs = t2.eval xs
| and p q => p.denote xs ∧ q.denote xs
| or p q => p.denote xs ∨ q.denote xs

Comment on lines +827 to +828
theorem Predicate.toFsm_correct {k : Nat} (p : Predicate k) :
decideIfZeros p.toFSM = true ↔ p.denote := by sorry
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similarly, this needs to be:

theorem Predicate.toFsm_correct {k : Nat} (p : Predicate k) :
  decideIfZeros p.toFSM = true ↔ (\all xs, p.denote xs) := by sorry

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.

4 participants