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

Add optimization pass for initial workspace #1521

Merged
merged 13 commits into from Oct 28, 2016

Conversation

Projects
None yet
3 participants
@llllllllll
Member

llllllllll commented Oct 4, 2016

Also adds an alias method of terms which allows people to "name" an expression which makes it repr better.

@llllllllll llllllllll force-pushed the add-optimization-pass-for-initial-workspace branch from 856531a to 09f5fb9 Oct 4, 2016

@coveralls

This comment has been minimized.

coveralls commented Oct 4, 2016

Coverage Status

Coverage decreased (-0.08%) to 86.795% when pulling 09f5fb9 on add-optimization-pass-for-initial-workspace into 7fc39fe on master.

@coveralls

This comment has been minimized.

coveralls commented Oct 4, 2016

Coverage Status

Coverage decreased (-0.1%) to 86.754% when pulling ad9120c on add-optimization-pass-for-initial-workspace into 7fc39fe on master.

@coveralls

This comment has been minimized.

coveralls commented Oct 4, 2016

Coverage Status

Coverage decreased (-0.07%) to 86.797% when pulling ad9120c on add-optimization-pass-for-initial-workspace into 7fc39fe on master.

@llllllllll llllllllll force-pushed the add-optimization-pass-for-initial-workspace branch from beffedd to 0fc9e79 Oct 7, 2016

@coveralls

This comment has been minimized.

coveralls commented Oct 8, 2016

Coverage Status

Coverage decreased (-0.007%) to 86.796% when pulling 7c68655 on add-optimization-pass-for-initial-workspace into eba02da on master.

@llllllllll llllllllll force-pushed the add-optimization-pass-for-initial-workspace branch from 7c68655 to f4bad5b Oct 10, 2016

@coveralls

This comment has been minimized.

coveralls commented Oct 10, 2016

Coverage Status

Coverage decreased (-0.008%) to 86.867% when pulling f4bad5b on add-optimization-pass-for-initial-workspace into fe00452 on master.

from .base import BasePipelineTestCase


class WithAlias(object):

This comment has been minimized.

@ssanderson

ssanderson Oct 11, 2016

Member

I would probably inherit from BasePipelineTestCase in WithAlias, since this is where we're actually defining the functionality that depends on the superclass.

This comment has been minimized.

@llllllllll

llllllllll Oct 11, 2016

Member

This is not actually a test though, just a local fixture. That is why I chose the With* naming convention.

This comment has been minimized.

@ssanderson

ssanderson Oct 11, 2016

Member

You can decorate with nottest to make nose not run the base class. IMO, if you're defining tests methods in a class, you're no longer a fixture, you're a base class for other tests.

At the very least, I'd put an init_class_fixtures here asserting that any instantiated subclass of this is also a subclass of BasePipelineTestCase, since the tests here will fail in a non-obvious way if a consumer doesn't inherit from both.

)

def dispatcher(column):
if column is base_term:

This comment has been minimized.

@ssanderson

ssanderson Oct 11, 2016

Member

base_term should never be loaded no matter what because it's a Factor, which isn't a LoadableTerm. I think you want to be checking against column here.

def test_populate_default_workspace(self):
column = USEquityPricing.low
base_term = column.latest
term = (base_term + 1).alias('term')

This comment has been minimized.

@ssanderson

ssanderson Oct 11, 2016

Member

Can we call this something like precomputed_term?

column = USEquityPricing.low
base_term = column.latest
term = (base_term + 1).alias('term')
composed_term = term + 1

This comment has been minimized.

@ssanderson

ssanderson Oct 11, 2016

Member

Similarly, I'd call this something like depends_on_precomputed_term.

This is the inverse of :func:`~zipline.pipeline.term.Term.postprocess`.
"""
data = super(Classifier, self).unprocess(result, assets)

This comment has been minimized.

@ssanderson

ssanderson Oct 11, 2016

Member

Where is unprocess defined? I don't see that anywhere. Is this code path tested?

This comment has been minimized.

@llllllllll

llllllllll Oct 11, 2016

Member

This was the old name for this method, I will add a test for this.

A function which will be used to populate the initial workspace when
computing a pipeline. This function will be passed the
initial_workspace, the root mask term, the execution_plan, the dates
being computed for, and the assets requested and should return a new

This comment has been minimized.

@ssanderson

ssanderson Oct 11, 2016

Member

An easier way to document this might be to write a numpy-style docstring on default_compute_initial_workspace, and point the reader there for an example.

This comment has been minimized.

@ssanderson

ssanderson Oct 11, 2016

Member

The docs for this should probably note that any pre-computed terms should have a shape given by execution_plan.mask_and_dates_for_term.

),
))

def ordered(self):

This comment has been minimized.

@ssanderson

ssanderson Oct 11, 2016

Member

I think this is unused now.

This comment has been minimized.

@llllllllll

llllllllll Oct 12, 2016

Member

This is used in test_term on graphs where we don't have or care about refcounts. I think it is fine to leave it.


return refcounts

def _decref_recursive(self, term, refcounts, garbage):
"""
Decrement terms recursivly to build the initial workspace.

This comment has been minimized.

@ssanderson

ssanderson Oct 11, 2016

Member

typo "recursively"

This comment has been minimized.

@ssanderson

ssanderson Oct 11, 2016

Member

This might benefit from a description of when this should be used versus decref_dependencies. I think the distinction is that this should be used when computing initial refcounts on the graph from a set of precomputed terms, whereas decref_dependencies should be used during computation to decrement a term that was just computed.


return refcounts

def _decref_recursive(self, term, refcounts, garbage):

This comment has been minimized.

@ssanderson

ssanderson Oct 11, 2016

Member

I'd probably name this _decref_dependencies_recursive, since it doesn't actually decref term.

refcounts[parent] -= 1
# No one else depends on this term. Remove it from the
# workspace to conserve memory.
if refcounts[parent] == 0:
garbage.add(parent)
return garbage

def __iter__(self):

This comment has been minimized.

@ssanderson

ssanderson Oct 11, 2016

Member

Do we depend on having an __iter__ somewhere?

mask = term.mask
mask_offset = self.extra_rows[mask] - self.extra_rows[term]

# This offset is computed against _root_mask_term because that is what

This comment has been minimized.

@ssanderson

ssanderson Oct 11, 2016

Member

This comment doesn't need an _ in the name anymore.


return type(
'Aliased' + other_base.__name__,
(cls, other_base,),

This comment has been minimized.

@ssanderson

ssanderson Oct 11, 2016

Member

Extra comma in the tuple here.

@@ -20,6 +20,7 @@
from zipline.utils.input_validation import expect_types
from zipline.utils.sharedoc import (
format_docstring,
PIPELINE_ALIAS_DOC,

This comment has been minimized.

@ssanderson

ssanderson Oct 11, 2016

Member

Can we call this PIPELINE_ALIAS_NAME_DOC? I read this name as suggesting that it holds the entire docstring for Alias, which makes it confusing when it's templated in for just the name parameter.

@@ -1315,3 +1318,58 @@ def test_string_classifiers_produce_categoricals(self):
columns=self.asset_finder.retrieve_all(self.asset_finder.sids),
)
assert_frame_equal(result.c.unstack(), expected_final_result)


class PopulateInitialWorkspaceTestCase(WithConstantInputs, ZiplineTestCase):

This comment has been minimized.

@ssanderson

ssanderson Oct 11, 2016

Member

It looks like we're only testing terms with window_lengths of 0 right now. Can we add a test for aliasing a windowed term? An example of such a term would be an instance of SimpleMovingAverage.

@ssanderson

This comment has been minimized.

Member

ssanderson commented Oct 11, 2016

@llllllllll finished this round. Mostly looks good to me. Had a few small notes, mostly around documentation and name choices.

@ssanderson

This comment has been minimized.

Member

ssanderson commented Oct 13, 2016

Flake8 failing:

zipline/testing/predicates.py:494:1: E303 too many blank lines (3)
tests/pipeline/test_classifier.py:523:22: E225 missing whitespace around operator
tests/pipeline/test_engine.py:37:1: F401 'assoc' imported but unused
@coveralls

This comment has been minimized.

coveralls commented Oct 14, 2016

Coverage Status

Coverage decreased (-0.002%) to 86.873% when pulling 412870a on add-optimization-pass-for-initial-workspace into fe00452 on master.

@llllllllll llllllllll force-pushed the add-optimization-pass-for-initial-workspace branch from 4ee4221 to cfae08f Oct 18, 2016

@llllllllll llllllllll force-pushed the add-optimization-pass-for-initial-workspace branch from cfae08f to 52cb26d Oct 28, 2016

@coveralls

This comment has been minimized.

coveralls commented Oct 28, 2016

Coverage Status

Coverage increased (+0.02%) to 86.998% when pulling 52cb26d on add-optimization-pass-for-initial-workspace into 285b5f7 on master.

@llllllllll llllllllll force-pushed the add-optimization-pass-for-initial-workspace branch from 52cb26d to ba1a47f Oct 28, 2016

@coveralls

This comment has been minimized.

coveralls commented Oct 28, 2016

Coverage Status

Coverage increased (+0.02%) to 86.998% when pulling ba1a47f on add-optimization-pass-for-initial-workspace into 2c819a6 on master.

@llllllllll llllllllll force-pushed the add-optimization-pass-for-initial-workspace branch from ba1a47f to dd191f4 Oct 28, 2016

@coveralls

This comment has been minimized.

coveralls commented Oct 28, 2016

Coverage Status

Coverage increased (+0.02%) to 86.994% when pulling dd191f4 on add-optimization-pass-for-initial-workspace into 5ecb5c3 on master.

@llllllllll llllllllll merged commit 156678a into master Oct 28, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@llllllllll llllllllll deleted the add-optimization-pass-for-initial-workspace branch Oct 28, 2016

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