-
Notifications
You must be signed in to change notification settings - Fork 25
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
Fix adding many clauses #44
Conversation
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.
Wow, I'm so sorry. I should have had a test, too. I feel ashamed. Thank you! ❤️
BTW, do you know how to run the python test cases as part of the Python package build? |
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. |
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 |
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 ( pip install ../.. && ./test_pyapproxmc.py |
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! |
Looks good to me 👍 thanks |
Glad to hear! :) |
The support for adding many clauses using the
add_clauses
method in the Python bindings did not work since the clauses where added to theself->appmc
object instead of theself->arjun
object. (The code foradd_clause
only interacts with theself->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 of512 * 2**90
. This could be because I'm running on an ARM machine. Anyway, I adapted the test case to compare the final total.