meelgroup / approxmc

Approximate Model Counter
Other
70 stars 25 forks source link

Fix adding many clauses #44

Closed fanosta closed 10 months ago

fanosta commented 10 months ago

The support for adding many clauses using the add_clauses method in the Python bindings did not work since the clauses where added to the self->appmc object instead of the self->arjun object. (The code for add_clause only interacts with the self->arjun object.) This pull request fixes that.

Also in the process of debugging the issue, I created two new test cases and converted the Python test suite to use pytest which makes it a bit nicer in my opinion.

With the test cases I noticed that the minimal test case was failing on my machine because ApproxMC returned 64 * 2**93 instead of 512 * 2**90. This could be because I'm running on an ARM machine. Anyway, I adapted the test case to compare the final total.

msoos commented 10 months ago

BTW, do you know how to run the python test cases as part of the Python package build?

msoos commented 10 months ago

I think it needs a line here somewhere: https://github.com/meelgroup/approxmc/actions/runs/7288050825/workflow But for the life of me I can't figure out what. Sorry, I'm SUCH a newbie to Python, I feel ashamed.

msoos commented 10 months ago

Also, how did YOU run the tests on your machine? I know this sounds crazy but it's so incredibly convoluted to do that. I'd need to do the python -m build, then install the module, and then I could run the test. But that sounds absolutely insane. There must be a better way... Again, I'm showing my stupidity here. Can you tell me how you build & debug this package? I have been having trouble since I moved to this new build system to build & debug in an effective way. The new python build system is very nice because it generates all the wheels that are independent of everything, but it also has lead to a significant degradation of my understanding of how to test & debug this thing.

fanosta commented 10 months ago

No worries thank you for merging the fixes 😊

I'll see if I can integrate running the tests into the GH action. Packaging Python modules with native code is always a mess.

Speaking of the build process I think something went wrong there. When I install the latest version from PyPI, I see the updated version number but the behaviour is still like before the merge requests.

>>> import pyapproxmc
>>> c = pyapproxmc.Counter()
>>> pyapproxmc.VERSION
'4.1.22'
>>> c.count()
(1, 0)
>>> c.count()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
SystemError: ERROR: Sampling vars contain variables that are not in the original clauses!

(The error message was changed in #45)

When debugging and running the test cases, I mainly used the setup as described in python/README.md. Only thing I changed is to use a separate virtualenv (python3 -m venv venv/; source venv/bin/activate) and run the thing in one command like so

pip install ../.. && ./test_pyapproxmc.py
msoos commented 10 months ago

Ah I see. I have now pushed a new version, hopefully the new module in pypi will be correct now. I'll check later. Sorry for that and thank you so much for your help!

fanosta commented 10 months ago

Looks good to me 👍 thanks

msoos commented 10 months ago

Glad to hear! :)