-
Notifications
You must be signed in to change notification settings - Fork 2
add new minimizer and fix bug #44
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
base: main
Are you sure you want to change the base?
Conversation
fix scalar-pla convert
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #44 +/- ##
==========================================
- Coverage 52.84% 50.23% -2.61%
==========================================
Files 51 51
Lines 3151 3354 +203
==========================================
+ Hits 1665 1685 +20
- Misses 1486 1669 +183 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@@ -1,4 +1,5 @@ | |||
""" | |||
by https://jackhack96.github.io/logic-synthesis/espresso.html. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docstrings should start with the function signature, not anything else. Let's could cite the reference in the docstring text body
src/espresso.linux | ||
src/abc | ||
src/boom.exe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either we make these part of the package (but let's ensure we can and there are no licensing issues), or we introduce some logic for downloading them automatically. If the executable are not too large (< 5MB) I would lean towards the first choice. Btw they should be executable on linux machines surely (idk about that exe
file?). Also what's the .linux
extension? Is it an executable?
- The function assumes that either the main condition or its dual exists in the conditions vector | ||
- Warnings are issued when logical conflicts are detected during encoding | ||
- The resulting PLA row uses "-" for don't-care positions that are not constrained by any literal | ||
""" | ||
function encode_disjunct(disjunct::LeftmostConjunctiveForm, features::AbstractVector, conditions::AbstractVector, includes, excludes, feat_condindxss) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is a bit of a mess (my bad, sorry, I've rushed a bit while writing it). It's difficult for me atm to understand what's going on and whether the fix is correct. Do all the tests pass?
Update on Logical Minimizers and Bug Fix
I started by adding two new logical minimizers,
boom
andabc
. While working on this, I noticed a bug in the conversion functions between PLA and formulas. This led me to investigate further.Specifically, I was testing the correctness of the two core functions:
_formula_to_pla
_pla_to_formula
Both are located in the
SoleData/src/scalar-pla.jl
file under theSoleData
module.Here’s a minimal reproducible test case:
For most formulas, the
result
is logically equivalent to the original formulaf
, as expected. However, in this particular case, it’s not.Root Cause
I believe I found the issue:
The original implementation had a critical bug: it always overwrote values in
pla_row
without checking if they were already set, potentially causing logical conflicts.Fix
Key Improvement:
These two functions are extremely important — not just for
Lumen
, but also for a major feature I’ve been working on and was planning to demo inSoleData
.