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: Std.Sat.AIG #4953

Merged
merged 26 commits into from
Aug 12, 2024
Merged

feat: Std.Sat.AIG #4953

merged 26 commits into from
Aug 12, 2024

Conversation

hargoniX
Copy link
Contributor

@hargoniX hargoniX commented Aug 8, 2024

Step 2/~7 in upstreaming LeanSAT.

@hargoniX hargoniX requested review from TwoFX and digama0 as code owners August 8, 2024 07:52
@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc August 8, 2024 08:01 Inactive
@github-actions github-actions bot added the toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN label Aug 8, 2024
Copy link
Contributor

@tobiasgrosser tobiasgrosser left a comment

Choose a reason for hiding this comment

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

Very nicely prepared patch and a big step forward. 🥳 I left some stylistic comments.

src/Std/Sat/AIG/Basic.lean Outdated Show resolved Hide resolved
src/Std/Sat/AIG/Basic.lean Outdated Show resolved Hide resolved
src/Std/Sat/AIG/Basic.lean Outdated Show resolved Hide resolved
src/Std/Sat/AIG/Basic.lean Outdated Show resolved Hide resolved
src/Std/Sat/AIG/CNF.lean Outdated Show resolved Hide resolved
src/Std/Sat/AIG/RefStreamOperator/Map.lean Outdated Show resolved Hide resolved
src/Std/Sat/AIG/RefStreamOperator/Map.lean Outdated Show resolved Hide resolved
src/Std/Sat/AIG/RefStreamOperator/Map.lean Outdated Show resolved Hide resolved
src/Std/Sat/AIG/RelabelNat.lean Outdated Show resolved Hide resolved
src/Std/Sat/AIG/RelabelNat.lean Outdated Show resolved Hide resolved
@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc August 8, 2024 08:43 Inactive
@leanprover-community-mathlib4-bot
Copy link
Collaborator

leanprover-community-mathlib4-bot commented Aug 8, 2024

Mathlib CI status (docs):

  • ❗ Batteries CI can not be attempted yet, as the nightly-testing-2024-08-08 tag does not exist there yet. We will retry when you push more commits. If you rebase your branch onto nightly-with-mathlib, Batteries CI should run now. (2024-08-08 08:48:17)
  • ❗ Mathlib CI can not be attempted yet, as the nightly-testing-2024-08-08 tag does not exist there yet. We will retry when you push more commits. If you rebase your branch onto nightly-with-mathlib, Mathlib CI should run now. (2024-08-08 15:15:25)
  • 💥 Mathlib branch lean-pr-testing-4953 build failed against this PR. (2024-08-09 14:01:16) View Log
  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 30a52b794a39256361050c52fe0e41bbff91bc8d --onto dd6ed124baef64a26ef5860f49597fdcef371239. (2024-08-09 20:00:27)
  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 30a52b794a39256361050c52fe0e41bbff91bc8d --onto 5f31e938c1bf5bb2d6d2d29b26ea932ade115357. (2024-08-12 07:46:39)

Copy link
Contributor

@bollu bollu left a comment

Choose a reason for hiding this comment

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

Thanks for the super clean implementation! I've read upon CNF.lean, and will get to the others in a bit.

src/Std/Sat/AIG/Basic.lean Outdated Show resolved Hide resolved
src/Std/Sat/AIG/Basic.lean Outdated Show resolved Hide resolved
src/Std/Sat/AIG/Basic.lean Outdated Show resolved Hide resolved
src/Std/Sat/AIG/Basic.lean Outdated Show resolved Hide resolved
src/Std/Sat/AIG/Basic.lean Outdated Show resolved Hide resolved
src/Std/Sat/AIG/Basic.lean Outdated Show resolved Hide resolved
src/Std/Sat/AIG/Basic.lean Show resolved Hide resolved
src/Std/Sat/AIG/CNF.lean Outdated Show resolved Hide resolved
src/Std/Sat/AIG/CNF.lean Outdated Show resolved Hide resolved
@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc August 8, 2024 14:28 Inactive
@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc August 8, 2024 14:37 Inactive
src/Std/Sat/AIG/Basic.lean Outdated Show resolved Hide resolved
src/Std/Sat/AIG/Basic.lean Show resolved Hide resolved
src/Std/Sat/AIG/Basic.lean Outdated Show resolved Hide resolved
src/Std/Sat/AIG/Basic.lean Outdated Show resolved Hide resolved
src/Std/Sat/AIG/Basic.lean Outdated Show resolved Hide resolved
src/Std/Sat/AIG/If.lean Outdated Show resolved Hide resolved
src/Std/Sat/AIG/If.lean Outdated Show resolved Hide resolved
src/Std/Sat/AIG/Basic.lean Outdated Show resolved Hide resolved
src/Std/Sat/AIG/LawfulStreamOperator.lean Outdated Show resolved Hide resolved
src/Std/Sat/AIG/Lemmas.lean Outdated Show resolved Hide resolved
@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc August 8, 2024 15:08 Inactive
@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc August 8, 2024 15:38 Inactive
@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc August 8, 2024 15:54 Inactive
@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc August 9, 2024 12:59 Inactive
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Aug 9, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Aug 9, 2024
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added breaks-mathlib This is not necessarily a blocker for merging: but there needs to be a plan release-ci Enable all CI checks for a PR, like is done for releases labels Aug 9, 2024
@kim-em
Copy link
Collaborator

kim-em commented Aug 12, 2024

❤️

@kim-em kim-em added awaiting-author Waiting for PR author to address issues and removed awaiting-review Waiting for someone to review the PR labels Aug 12, 2024
@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc August 12, 2024 07:47 Inactive
@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc August 12, 2024 08:17 Inactive
@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc August 12, 2024 08:33 Inactive
@hargoniX hargoniX added awaiting-review Waiting for someone to review the PR and removed awaiting-author Waiting for PR author to address issues labels Aug 12, 2024
@hargoniX hargoniX requested a review from kim-em August 12, 2024 08:46
@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc August 12, 2024 08:50 Inactive
Copy link
Member

@TwoFX TwoFX left a comment

Choose a reason for hiding this comment

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

We could probably spend more time nitpicking here and there but I don't think there is much point, so let's get this merged after these final remarks.

src/Std/Sat/AIG/LawfulOperator.lean Outdated Show resolved Hide resolved
src/Std/Sat/AIG/Basic.lean Show resolved Hide resolved
Comment on lines 30 to 42
{
lhs := {
ref := constRef
inv := false
}
rhs := {
ref := gate.cast <| by
intros
apply LawfulOperator.le_size_of_le_aig_size (f := mkConstCached)
omega
inv := true
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I think there is no clear consensus in the existing Lean codebase at the moment (including mixing different styles within the same style), so I think in the interest of making progress I suggest leaving things as you have them now and not losing too much time on this as long as they adhere to one of these styles. However, neither style should have an opening brace on its own line (so this is not okay), and within a declaration you should stick to one style (so this is not okay).

@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc August 12, 2024 14:04 Inactive
@hargoniX hargoniX added will-merge-soon …unless someone speaks up and removed awaiting-review Waiting for someone to review the PR labels Aug 12, 2024
@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc August 12, 2024 14:42 Inactive
@hargoniX hargoniX added this pull request to the merge queue Aug 12, 2024
Merged via the queue into master with commit dc3eccd Aug 12, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaks-mathlib This is not necessarily a blocker for merging: but there needs to be a plan toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN will-merge-soon …unless someone speaks up
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants