search

pysathq / pysat

A toolkit for SAT-based prototyping in Python
https://pysathq.github.io/
MIT License
386 stars 69 forks source link

Very slow useless (?) deepcopy in from_clauses #157

Closed vincent-hugot closed 7 months ago

vincent-hugot commented 7 months ago

Hello,

I have experiments where most of the time is spent in a deepcopy which pysat does, but that seems to serve no purpose:

I do 

with Solver(bootstrap_with=CNF(from_clauses=m))
      ....

Pysat executes: https://github.com/pysathq/pysat/blob/5af91f840586782dfd651d11b5b5b32559b742f5/pysat/formula.py#L576

Profiling shows ungodly amounts of time spent in there; actually more time than I spent on my end generating the formula and converting it into CNF clauses in the first place

I don't care about preserving the clauses I generated (object m); if I replace the copy operation with

self.clauses = clauses

I still get the results I need, and shave a third or half off the time taken by my benchmark (depending on whether I'm profiling).

Have I missed an obvious way to bypass the deepcopy without altering pysat code?

If not, I'd hold that it is up to the user to know whether they care to preserve the input structure, and to do a deepcopy in that case, and that this shouldn't be forced upon them by pysat.

alexeyignatiev commented 7 months ago

Thanks for reporting, @vincent-hugot. I agree that it makes sense to let a user decide whether they want to copy the clauses or refer to the existing clauses object. I can add an argument by_ref=True (or False by default). I presume this should resolve this particular issue, shouldn't it?

alexeyignatiev commented 7 months ago

By the way, I am not sure why you are creating a CNF object here in the first place. You can just pass the clauses as the value given to bootstrap_with. That is why the deepcopy line was never a big deal to me: you typically either use a list of clauses or a CNF object created from scratch.

vincent-hugot commented 7 months ago

Thank you for your reactivity.

I can add an argument by_ref=True (or False by default). I presume this should resolve this particular issue, shouldn't it?

Yes, it would.

I am not sure why you are creating a CNF object here in the first place. [...]

... Because I did not read the documentation properly, it would seem. Passing m directly indeed removes the deepcopy tax. All is well.

I still think, on purely general grounds, that letting the user choose whether and when to deepcopy is a good policy, and if this specific issue, spurious as it was, served to highlight that, then my mistake was of some use.

Thank you again, and have a nice day. VH

alexeyignatiev commented 7 months ago

Just to let you know: this is now fixed in 0.1.8.dev16. :)