Skip to content

feat: sd-dnnf-checker - #24

Merged
jjcmoon merged 24 commits into
ML-KULeuven:mainfrom
IbrahimElk:feat/sd-dnnf-checker
May 7, 2026
Merged

feat: sd-dnnf-checker#24
jjcmoon merged 24 commits into
ML-KULeuven:mainfrom
IbrahimElk:feat/sd-dnnf-checker

Conversation

@IbrahimElk

Copy link
Copy Markdown
Contributor

This PR adds some property-checks for sd-DNNF circuits implemented using Klay.
Primarily, syntactic checks including decomposability and smoothness, and exposes the results to python.
This PR also adds tests covering these new check methods.

@IbrahimElk
IbrahimElk marked this pull request as draft March 27, 2026 14:10
@IbrahimElk

IbrahimElk commented Mar 27, 2026

Copy link
Copy Markdown
Contributor Author

I'll also clean up a bit the code in this PR, i have some commented out code that i should remove.
And also remove to_dot_file from the bindings in this PR that i needed in my personal branch for debugging.

* this adds a bottom-up checker that walks the circuit once, and checks
- decomposability: no variable appears in more than one child of an AND
- smoothness: every child of an OR mentions exactly the same variable scope

Signed-off-by: IbrahimElk <ibrahim_elkaddouri@outlook.com>
Signed-off-by: IbrahimElk <ibrahim_elkaddouri@outlook.com>
Signed-off-by: IbrahimElk <ibrahim_elkaddouri@outlook.com>
Signed-off-by: IbrahimElk <ibrahim_elkaddouri@outlook.com>
* exported symbols (e.g. check_sdnnf) are visible
  for linking and debugging

Signed-off-by: IbrahimElk <ibrahim_elkaddouri@outlook.com>
* this commit will be eventually reversed

Signed-off-by: IbrahimElk <ibrahim_elkaddouri@outlook.com>
Signed-off-by: IbrahimElk <ibrahim_elkaddouri@outlook.com>
Signed-off-by: IbrahimElk <ibrahim_elkaddouri@outlook.com>
Signed-off-by: IbrahimElk <ibrahim_elkaddouri@outlook.com>
@IbrahimElk
IbrahimElk force-pushed the feat/sd-dnnf-checker branch from 3ba6338 to ce22597 Compare April 16, 2026 09:18
- following stricter c++ guidelines:
  https://google.github.io/styleguide/cppguide.html#Namespaces

Signed-off-by: IbrahimElk <ibrahim_elkaddouri@outlook.com>
Signed-off-by: IbrahimElk <ibrahim_elkaddouri@outlook.com>
@IbrahimElk
IbrahimElk force-pushed the feat/sd-dnnf-checker branch from d606b48 to e7e8deb Compare April 16, 2026 14:32
enables safe subclassing of circuit from external consumers (e.g. kompyle).

Signed-off-by: IbrahimElk <ibrahim_elkaddouri@outlook.com>
Replace the determinism comment with a more accurate note: checking
determinism on arbitrary NNF circuits is coNP-complete.

Signed-off-by: IbrahimElk <ibrahim_elkaddouri@outlook.com>
Also drops the stale comment explaining why get_indices
is bound on Circuit.

Signed-off-by: IbrahimElk <ibrahim_elkaddouri@outlook.com>
Signed-off-by: IbrahimElk <ibrahim_elkaddouri@outlook.com>
Not all tests are run during ci.
Run all tests manually `pytest tests/* -vv -rs`

Signed-off-by: IbrahimElk <ibrahim_elkaddouri@outlook.com>
@IbrahimElk
IbrahimElk marked this pull request as ready for review April 24, 2026 14:25
@IbrahimElk

IbrahimElk commented Apr 24, 2026

Copy link
Copy Markdown
Contributor Author

@jjcmoon Would it be possible to bump the version when deciding to merge these changes ?
(such that the changes will be available in pypi)

@jjcmoon jjcmoon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this could simplified quite a bit, c.f. the comments.

Comment thread include/klay/properties.h Outdated
Comment thread tests/test_api.py
Comment thread include/klay/properties.h Outdated
Comment thread include/klay/circuit.h
std::vector<Node*> roots = {};

~Circuit() {
virtual ~Circuit() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why make Circuit virtual? I feel this isn't necessary.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's handy when inheriting the circuit class, although, it doesn't fit this PR,
I could move this to another PR if needed ?

In kompyle, for example, I inherit the circuit class in order to add an extra field,
but that fields doesn't fit in the klay project. So, i thought that would be the best solution ?

The field is to have a mapping from variable names to indices such that you can process
multiple non-cnf files into the same klay circuit and don't have the same name from different
files map to different indices in the same klay circuit.

Comment thread include/klay/properties.h Outdated
#include <string>
#include <vector>

#include <cassert>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Move unused headers.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

i have moved some to the cpp file, could you check if that is what you wanted ?
see commit 728e3b2

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

check latest commit instead or rather 8d8962a

Comment thread include/klay/properties.h Outdated

std::string sdnnf_summary(const SDNNFResult& r);

class IPropertyChecker {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This seems over-engineered. Can't you just make two methods that check decomposability and smoothness?

@IbrahimElk IbrahimElk May 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

sure, basically, have only the methods:

SDNNFResult check_decomposability(const Circuit& circuit, std::size_t max_violations);
SDNNFResult check_smooth(const Circuit& circuit, std::size_t max_violations);
SDNNFResult check_sdnnf(const Circuit& circuit, std::size_t max_violations);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

i have done what was ordered from me, am i free to go my lord ?

Comment thread include/klay/properties.h Outdated
IbrahimElk added 6 commits May 6, 2026 15:07
Signed-off-by: IbrahimElk <ibrahim_elkaddouri@outlook.com>
* gets automatically detected by pytest, which is used in CI

Signed-off-by: IbrahimElk <ibrahim_elkaddouri@outlook.com>
* removing unused code

Signed-off-by: IbrahimElk <ibrahim_elkaddouri@outlook.com>
* was used to check certain things when completing
  the search trough the klay circuit, but is now unused.

Signed-off-by: IbrahimElk <ibrahim_elkaddouri@outlook.com>
* moving certain dependencies to cpp

Signed-off-by: IbrahimElk <ibrahim_elkaddouri@outlook.com>
Signed-off-by: IbrahimElk <ibrahim_elkaddouri@outlook.com>
IbrahimElk added 2 commits May 6, 2026 16:09
Remove the IPropertyChecker interface, DecomposabilityChecker and
SmoothnessChecker classes, and the run_checks dispatcher in favour of
two static helpers (check_and_decomp, check_or_smooth) called directly
from check_decomposability, check_smooth and check_sdnnf.

Signed-off-by: IbrahimElk <ibrahim_elkaddouri@outlook.com>
Signed-off-by: IbrahimElk <ibrahim_elkaddouri@outlook.com>

@jjcmoon jjcmoon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the changes!

@jjcmoon
jjcmoon merged commit b3fd675 into ML-KULeuven:main May 7, 2026
1 check passed
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.

2 participants