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.

Sign up for GitHub

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

Jump to bottom

Add compress kwarg to Model.to_file() and States.to_file() #150

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add compress kwarg to Model.to_file() and States.to_file() #150

wants to merge 1 commit into from

Conversation

arcondello
Copy link
Member

By default this maintains the existing behavior but allows us to toggle it via a keyword argument.

See also dwavesystems/dimod#1296

@arcondello arcondello added the enhancement New feature or request label Oct 25, 2024
@arcondello arcondello marked this pull request as ready for review October 25, 2024 21:00
randomir
randomir approved these changes Oct 25, 2024
View reviewed changes
Copy link
Member

@randomir randomir left a comment

Choose a reason for hiding this comment

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

LGTM.

It would be nice to expose some compression level controls as well, or at the very least to specify explicitly what's used (zlib.Z_DEFAULT_COMPRESSION/6).

dwave/optimization/model.pyx
@@ -427,6 +427,7 @@ cdef class Model:
def into_file(self, file, *,
Py_ssize_t max_num_states = 0,
bool only_decision = False,
bool compress = False,
Copy link
Member

Choose a reason for hiding this comment

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

How about we expose compression level as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about that, but erred on consistency with dwavesystems/dimod#1296 and the others for compatibility. No one has requested finer control for those so 🤷

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense. I can see pros and cons of both approaches.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like someone already did the analysis. 🤦

@arcondello
Copy link
Member Author

On the one hand, this is a pretty harmless PR, on the other I am underwhelmed with the results. A simple benchmark

10000 binary variables in a single array with 25 states, uncompressed
serialization time: 0.006702525999571662s
file size in bytes: 2007953 bytes
10000 binary variables in a single array with 25 states, compressed
serialization time: 0.06300492599984864s
file size in bytes: 83618 bytes
10000 binary variables as 10000 scalars with 25 states, uncompressed
serialization time: 9.95059110299917s
file size in bytes: 74242127 bytes
10000 binary variables in a single array with 25 states, compressed
serialization time: 13.670800387000781s
file size in bytes: 57585889 bytes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@randomir randomir randomir approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@arcondello @randomir