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

Fix adding many clauses #44

Merged
merged 3 commits into from
Dec 21, 2023
Merged

Conversation

fanosta
Copy link
Contributor

@fanosta fanosta commented Dec 21, 2023

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.

Copy link
Collaborator

@msoos msoos left a 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! ❤️

@msoos
Copy link
Collaborator

msoos commented Dec 21, 2023

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

@msoos
Copy link
Collaborator

msoos commented Dec 21, 2023

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
Copy link
Collaborator

msoos commented Dec 21, 2023

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.

@msoos msoos merged commit a40eb45 into meelgroup:master Dec 21, 2023
5 checks passed
@fanosta
Copy link
Contributor Author

fanosta commented Dec 22, 2023

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
Copy link
Collaborator

msoos commented Dec 22, 2023

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
Copy link
Contributor Author

fanosta commented Dec 24, 2023

Looks good to me 👍 thanks

@msoos
Copy link
Collaborator

msoos commented Dec 24, 2023

Glad to hear! :)

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