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

various changes to path machinery #61

Merged
merged 6 commits into from Oct 2, 2018

Conversation

@jcmgray
Copy link
Collaborator

commented Oct 1, 2018

Description

In the process of comparing/benchmarking the new 'cheap' path (#60) I thought various consistency/convenience updates to the path machinery might be worthwhile. Some of these are bigger than others so definitely let me know what you think.

  1. Per #55, change the memory_limit default to unlimited. As I mentioned in the other thread, I think any time that the arrays are going to get too big for RAM, resorting to einsum is going to be so exponentially slow that a MemoryError might be preferable. We could add 'warn'/'system' versions that take system memory into account, the only thing is that this requires checking the output dtype and getting the systems free memory which is a bit of overhead. This change does make 'optimal' a bit slower, but at the same time, it possibly wasn't obvious previously that the path found might not have been globally optimal. Happy to split/delay this into a later PR if necessary (@dgasmith?).

  2. Deprecated (with a warning) the path= keyword argument in contract_path in favour of optimize=. Other functions, including in numpy use optimize so I think this makes sense to have a matching signature between contract, contract_path, & contract_expression.

  3. Factor the full path information into a class (PathInfo) and update it to allow printing flop costs >1e307 (which previously errored). This allows access to the various path costs programatically (e.g. path.opt_cost, path.largest_intermediate etc) and makes the default repr the full printed info so no need to print(path). This is a breaking change if anyone was using the path as a proper string (e.g. @fritzo I saw you were regexing this) but ultimately should negate the need to do that (and a cheap fix is just str(path)).

  4. Add helper function oe.helpers.rand_equation. This is a just a private function that can be helpful to generate large random expressions with variable connectivity and is thus might be useful for testing.

  5. Finally, the cupy tests weren't working - I think it needs to be imported explicitly, and before other GPU libs, in order to initialize CUDA properly, so I've done that.

TODO:

  • Note the memory_limit choice in the docs somewhere?
  • Test the rand_equation generator / update some tests to use it?

Status

  • Ready to go
fix cupy a bit
@codecov-io

This comment has been minimized.

Copy link

commented Oct 1, 2018

Codecov Report

Merging #61 into master will decrease coverage by 0.16%.
The diff coverage is 91.46%.

Copy link
Contributor

left a comment

👍 It will be great to have access to the PathInfo object!

path_run = (self.scale_list[n], do_blas, einsum_str, remaining_str)
path_print += "\n{:>4} {:>14} {:>22} {:>37}".format(*path_run)

return path_print

This comment has been minimized.

Copy link
@fritzo

fritzo Oct 1, 2018

Contributor

nit: you could avoid quadratic growth by using lines.append(...) and finally return '\n'.join(lines)

@@ -15,6 +15,68 @@
__all__ = ["contract_path", "contract", "format_const_einsum_str", "ContractExpression", "shape_only", "shape_only"]


class PathInfo:

This comment has been minimized.

Copy link
@fritzo

fritzo Oct 1, 2018

Contributor

nit: Python 2 convention is to always inherit from object:

class PathInfo(object):
    ...
@jcmgray

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 1, 2018

@fritzo, great, I've made those couple of changes - thanks for pointing them out.

Copy link
Owner

left a comment

Overall, LGTM! Thanks for the changes.

" Optimized FLOP count: {:.3e}\n".format(opt_cost),
" Theoretical speedup: {:3.3f}\n".format(speedup),
" Largest intermediate: {:.3e} elements\n".format(largest_intermediate),
"-" * 80 + "\n",

This comment has been minimized.

Copy link
@dgasmith

dgasmith Oct 1, 2018

Owner

+1 for format. If you thinking about it can we replace the old % syntax elsewhere through the code so we can be consistent? Ancillary point, we can spin this off in its own issue.

path_type = kwargs.pop('path', 'auto')
if 'path' in kwargs:
import warnings
warnings.warn("The 'path' keyword argument is deprecated in favor of 'optimize'.", DeprecationWarning)

This comment has been minimized.

Copy link
@dgasmith

dgasmith Oct 1, 2018

Owner

Agree, this is a good change overall.

By default (None) will size the ``memory_limit`` as the largest input tensor.
Users can also specify ``-1`` to allow arbitrarily large tensors to be built.
- if None or -1, there is no limit.

This comment has been minimized.

Copy link
@dgasmith

dgasmith Oct 1, 2018

Owner

Can we change the text style here to follow more like:

- ‘None’ or -1 means there is no limit
- ‘max_input’ means the limit is set as the size of the largest input tensor
...
The default is `None`.
Examples
--------
>>> eq, shapes = rand_equation(n=10, reg=4, n_outer=5, seed=42)

This comment has been minimized.

Copy link
@dgasmith

dgasmith Oct 1, 2018

Owner

Should be rand_equation(10, 4, 5, seed=42) I think?

This comment has been minimized.

Copy link
@jcmgray

jcmgray Oct 1, 2018

Author Collaborator

This works fine for me leaving them all as keyword arguments?

reg : int
Average connectivity of graph.
n_outer : int
Number of outer indices.

This comment has been minimized.

Copy link
@dgasmith

dgasmith Oct 1, 2018

Owner

Can you expend on outer indices?

n : int
Number of array arguments.
reg : int
Average connectivity of graph.

This comment has been minimized.

Copy link
@dgasmith

dgasmith Oct 1, 2018

Owner

Not sure what reg is meant to symbolize here, perhaps write this word out?

@@ -167,3 +168,95 @@ def flop_count(idx_contraction, inner, num_terms, size_dictionary):
op_factor += 1

return overall_size * op_factor


def rand_equation(n, reg, n_outer, dmin=2, dmax=9, seed=None):

This comment has been minimized.

Copy link
@dgasmith

dgasmith Oct 1, 2018

Owner

There might be a slight mismatch between styles here dmin nouter vs d_min, n_outer. I think your current nomenclature is the lesser of the evils, but worth thinking about.

@dgasmith

This comment has been minimized.

Copy link
Owner

commented Oct 1, 2018

It might be good to write a few tests that use the random path tech with a static seed and compares that the results are the same across all paths.

@fritzo fritzo referenced this pull request Oct 1, 2018
5 of 5 tasks complete
jcmgray added 3 commits Oct 1, 2018
@pytest.mark.parametrize("reg", [3, 4])
@pytest.mark.parametrize("n_out", [0, 2, 4])
def test_rand_equation(optimize, n, reg, n_out):
eq, shapes = helpers.rand_equation(n, reg, n_out, d_min=2, d_max=5)

This comment has been minimized.

Copy link
@dgasmith

dgasmith Oct 1, 2018

Owner

Did we want to add a seed to prevent irreducible tests? Alternatively, we could leave the random tests in as long as we print the seed on failure so we can reproduce later.

This comment has been minimized.

Copy link
@jcmgray

jcmgray Oct 1, 2018

Author Collaborator

Thanks for spotting I missed this - fixed now.

@dgasmith

This comment has been minimized.

Copy link
Owner

commented Oct 1, 2018

As mentioned elsewhere I think we should try to get this in before #60.

Copy link
Owner

left a comment

I think this looks good to go once the tests complete. Let me know if there are any additional holdups.

@jcmgray

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 2, 2018

Great I'll merge.

@jcmgray jcmgray merged commit ccbdf6b into dgasmith:master Oct 2, 2018
3 checks passed
3 checks passed
LGTM analysis: Python No alert changes
Details
codecov/project 96.44% (-0.17%) compared to 1db5a0a
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dgasmith dgasmith added this to the v2.3 milestone Oct 3, 2018
@dgasmith dgasmith added the enhancement label Oct 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.