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

Refactoring & optimizing large BinaryPolynomial construction #992

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
+2,176 −22
Open

Refactoring & optimizing large BinaryPolynomial construction #992

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
050b51f
refactor and optimize BinaryPolynomial.__init__
mhlr Sep 10, 2021
0452df7
(temporarily) adding notbook demnstrating >6x speedup with new __init…
mhlr Sep 10, 2021
b209990
update evaluation notebook
mhlr Sep 10, 2021
7ddbe4c
simplify freeze_term
mhlr Sep 10, 2021
6c4cc54
Merge branch 'dwavesystems:main' into binary-polynomial
mhlr Sep 11, 2021
727ca81
Merge branch 'dwavesystems:main' into binary-polynomial
mhlr Sep 13, 2021
44e0921
added reduce_terms
mhlr Sep 21, 2021
6923abd
new version of make_quadratic - make_queadratic_old temporarily retained
mhlr Sep 21, 2021
695b45a
bugfix
mhlr Sep 21, 2021
887dec0
simpolifications
mhlr Sep 21, 2021
679e15f
more simplification
mhlr Sep 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 25 additions & 21 deletions dimod/higherorder/polynomial.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,14 @@

import itertools
import collections.abc as abc
from collections import defaultdict

from numbers import Number

import numpy as np

import cytoolz as tl
Copy link
Member

@arcondello arcondello Sep 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

while cytoolz is a cool package, I don't think we can add it as a core ocean dependency. It seems supported-ish, but, for instance, it only started releasing wheels in august/september.

If/when it matures more, then we can consider adding it.

Copy link
Contributor Author

@mhlr mhlr Sep 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Fair enough. Cytoolz has been in conda since June 2016 though
Many science/ai/ml package are conda only/first.

I am also using these PRs to explore ideas even if they do get merged. Hope that is ok

Copy link
Member

Choose a reason for hiding this comment

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

More than ok! It's much easier to have these conversations on a PR.

For conda, we actually have an issue about this dwavesystems/dwave-ocean-sdk#144. I do think conda support in addition would be nice, but we also need to support pypi installs.

Copy link
Contributor Author

@mhlr mhlr Sep 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Great! I would love to have dwave code in conda. Outside dwave I use conda almost exclusively. One adwantage is that the offical repos are curated so the quality tends to be higher than pypi. Also the conda (& now also mamba) package manager maintains global environment consistecy so one install does not break previously installed packages. Conda will (up/down) grade previouly installed packages if necessary. Conda is also language agnostic so you can distribute C, R, Julia, Rust packages with it. Main drawback is it can not install directly from git.

The reason I brought cond up though was more that cytoolz is more active/mature/maintained than it's pypi history would suggest. I believe dask was also long time in conda before it got released on pypi.

Otherwise I think cytoolz & conda are independent issues

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also conda gives you MKL compiled numerical libraries by default, which in turn means that native vectorised numpy/scipy operation run multithreaded by default (without GIL)

Copy link
Contributor Author

@mhlr mhlr Sep 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It also looks like cytool zhas been in PiPy since 2014 https://pypi.org/project/cytoolz/#history

Copy link
Member

Choose a reason for hiding this comment

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

Yes, though they only released an sdist until september. Which means that users without python-dev installed would not be able to install Ocean.
We have to be extremely careful about introducing new dependencies, especially ones with compiled code. Because they need to work across all of our supported OS/python versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I did not realize that the wheel availability was the issue. I thought the main concern was general maturity & support. If they are releasing wheels now what is the criterion for acceptance? Some period of time or some number of wheel releases?

Copy link
Member

Choose a reason for hiding this comment

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

There isn't a hard algorithm. But experience over the last few years has taught us to be extremely cautious when introducing new dependencies to Ocean. Especially compiled dependencies. A significant fraction of our support requests come from dependency management. Another place this comes up, each time Python releases a new version we can only release a supporting version of Ocean once all of our dependencies also update. So for some packages like NumPy, they are so fundamental that we cannot possibly avoid using them. But otherwise we definitely err, pretty far, on keeping our dependency tree as small as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough. I do think that (cy)toolz is the kind universal nuts&bolts library linke numpy, scipy & pandas that is worth considering since it can be used everywhere and can reduces the need for resorting to C & cython


from dimod.decorators import vartype_argument
from dimod.sampleset import as_samples
from dimod.utilities import iter_safe_relabels
Expand All @@ -34,6 +37,22 @@ def asfrozenset(term):
return term if isinstance(term, frozenset) else frozenset(term)


def freeze_term(term, vartype):
return (
term
if isinstance(term, frozenset)
else (
frozenset(term)
if vartype is Vartype.BINARY
else frozenset(
var
for var, power in tl.frequencies(term).items()
Copy link
Member

Choose a reason for hiding this comment

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

Seems like a big dependency to bring in for a 4 line function.

Copy link
Member

Choose a reason for hiding this comment

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

I also suspect that in typical use cases, i.e. relatively low degree terms, that just doing tuple.count() will be faster than constructing a new defaultdict.

Copy link
Member

Choose a reason for hiding this comment

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

In [7]: term = tuple('abcde')

In [8]: %timeit for _ in frequencies(term): pass
1.31 µs ± 9.02 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

In [9]: %timeit for _ in ((v, term.count(v)) for v in term): pass
979 ns ± 7.79 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

Copy link
Member

Choose a reason for hiding this comment

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

I also wonder if Counter would do the same thing.

Copy link
Contributor Author

@mhlr mhlr Sep 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

That result seems to be from the interpreted toolz version of frequencies. The compiled cytools version is >2x faster than the .count version. The count version need to iterate over set(terms) rather than term which slows it down more, making it closer to 3x slower than compiled cytoolz.
Counter is around the same speed as the interpreted version of frequencies and provides the same syntactic benefit.

%timeit for _ in toolz.frequencies(term).items(): pass
    1.16 µs ± 23.5 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

%timeit for _ in ((v, term.count(v)) for v in term): pass
    826 ns ± 46.9 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

%timeit for _ in cytoolz.frequencies(term).items(): pass
    347 ns ± 10.5 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

%timeit for _ in ((v, term.count(v)) for v in set(term)): pass
    1.01 µs ± 49.5 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

%timeit for _ in Counter(term).items(): pass
    1.08 µs ± 20.2 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

Copy link
Contributor Author

@mhlr mhlr Sep 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Seems like a big dependency to bring in for a 4 line function.

Cytoolz offers 2 benefits: clean code and speed - the original interpreted toolz library only provides the code quality part
The speed cytoolz brings is often sufficient to avoid needing to cythonize or port code to C leading to further code quality benefits as well
Depending on it brings in a moderately large number of small cythonized utility functions only a small number of which is likely to be used in any given PR. In each case it is easy to either reimplement an equivalent function or find a work around. In the long run though that means either having most of the library reimplemented possibly multiple times throughout our codebase and/or a lot of workarounds. Individual reimplementations are unlikely to get cythonized, so the codebase misses out on the performance benefit, in some cases leading to unnecessary C ports.

This could by demonstrated by refactoring a large piece of code that is performance bottleneck. I will do more refactoring and optimization in this file for now to see how many more cytoolz uses turn up. So far i have been doing it method by method to keep the PRs small but a combine PR would demonstrate the cumulative effect

if power % 2
)
)
)


class BinaryPolynomial(abc.MutableMapping):
"""A polynomial with binary variables and real-valued coefficients.

Expand Down Expand Up @@ -92,26 +111,11 @@ def __init__(self, poly, vartype):
poly = poly.items()

# we need to aggregate the repeated terms
self._terms = terms = {}
for term, bias in poly:

fsterm = asfrozenset(term)

# when SPIN-valued, s^2 == 1, so we need to handle that case
# in BINARY, x^2 == x
if len(fsterm) < len(term) and vartype is Vartype.SPIN:
new = set()
term = tuple(term) # make sure it has .count
for v in fsterm:
if term.count(v) % 2:
new.add(v)
fsterm = frozenset(new)

if fsterm in terms:
terms[fsterm] += bias
else:
terms[fsterm] = bias

self._terms = terms = defaultdict(int)
for term, bias in poly:
terms[freeze_term(term, vartype)] += bias

self.vartype = vartype

def __contains__(self, term):
Expand All @@ -127,10 +131,10 @@ def __eq__(self, other):
except Exception:
# not a polynomial
return False

Copy link
Member

Choose a reason for hiding this comment

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

not sure why these lines needed changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like an eccidental whitespace change. Will fix. I am not seeing any semantic changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we normally have whitespace in blank lines up to indentation level? Seems like that is what I removed

Copy link
Member

Choose a reason for hiding this comment

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

No, in fact removing the whitespace is correct. It's more about keeping the blame/commit history clean. No big deal either way.

self_terms = self._terms
other_terms = other._terms

return (
self.vartype == other.vartype
and all(
Expand Down
Loading