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

BUG Forward initialize args and kwargs to user-defined function. #687

Merged
merged 1 commit into from Sep 14, 2015

Conversation

Projects
None yet
4 participants
@twiecki
Contributor

twiecki commented Aug 25, 2015

No description provided.

@jfkirk

This comment has been minimized.

Contributor

jfkirk commented Aug 25, 2015

LGTM - have you checked qexec compatibility? I don't know of anything explicit that would conflict, but I'm not sure.

@twiecki twiecki force-pushed the forward_initialize_args branch from 4b5f5a5 to e9b8b78 Aug 25, 2015

@ssanderson

This comment has been minimized.

Member

ssanderson commented Aug 28, 2015

c.f. #634

@twiecki

This comment has been minimized.

Contributor

twiecki commented Sep 8, 2015

@jfkirk ping

@jfkirk jfkirk assigned StewartDouglas and unassigned jfkirk Sep 8, 2015

@StewartDouglas StewartDouglas force-pushed the forward_initialize_args branch 2 times, most recently from f6f2377 to 825beb7 Sep 10, 2015

@StewartDouglas

This comment has been minimized.

Contributor

StewartDouglas commented Sep 11, 2015

I don’t think we should be passing these args and kwargs to self._initialize.

self._initialize is set to whatever initialize(context) is in the algo code, which does not expect any args or kwargs. Consequently it will crash when we try to supply it with non-empty args and kwargs (which can happen, for example, when running algos with scripts/run_algo.py).

I’m looking at removing args and kwargs from the definition of the initialize method.

@twiecki

This comment has been minimized.

Contributor

twiecki commented Sep 11, 2015

I think the use-case is that you would define your own initialize(context, my_kwarg=5) so that you can pass in parameters externally.

I don't see when we would pass _args or *_kwargs when calling via run_algo.py?

@StewartDouglas

This comment has been minimized.

Contributor

StewartDouglas commented Sep 11, 2015

PYTHONPATH=. python scripts/run_algo.py -f zipline/examples/buyapple.py --start 2011-1-1 --end 2012-1-1

If you set a break point in def initialize you'll see that kwargs has the key 'namespace'. This could be resolved however by simply 'poping' namespace, rather than our current logic:

self.namespace = kwargs.get('namespace', {})

@StewartDouglas

This comment has been minimized.

Contributor

StewartDouglas commented Sep 11, 2015

Ok @twiecki I think we can incorporate your original suggestion, subject to me changing:

self.namespace = kwargs.get('namespace', {})
to
self.namespace = kwargs.pop('namespace', {})

in algorithm.py, and undoing a change I made to test_modelling_algo.py, which has since become unecessary after yesterday's big asset_writer merge (it passed an asset_finder to the TradingAlgorithm, which now does nothing other than cause self._initialize to barf).

Would you be ok if I make these changes and squash them into your original commit (to reduce commit noise and ensure that the test suite passes for this commit.)

@twiecki

This comment has been minimized.

Contributor

twiecki commented Sep 11, 2015

That sounds great.

To give a bit of context. This is really critical for parameter optimization and we have some great research that uses this feature.

@StewartDouglas StewartDouglas force-pushed the forward_initialize_args branch from 825beb7 to c131955 Sep 11, 2015

@StewartDouglas

This comment has been minimized.

Contributor

StewartDouglas commented Sep 11, 2015

@ssanderson mind taking a quick look over this, as it touches one of the tests your wrote (although it simply undoes some changes that I had previously made)

@StewartDouglas

This comment has been minimized.

Contributor

StewartDouglas commented Sep 11, 2015

Qexec unit tests passing but 3 integration tests failing (investigating, but I think these are unrelated failures)

@ssanderson

This comment has been minimized.

Member

ssanderson commented Sep 14, 2015

Assuming this is still passing downstream, LGTM.

BUG: Forward initialize args and kwargs to user-defined function.
The initialize method of TradingAlgorithm no longer accepts and
silently ignores args and kwargs, but instead forwards them
to the user-defined function referenced by self._initialize.

To avoid passing unexpected arguments to self._initialize, the
following additional adjustments are made:

 - pop 'namespace' from the kwargs supplied to TradingAlgorithm
   rather than simply get()ing it

 - do not pass an AssetFinder to the TradingAlgorithm in
   test_modelling_algo.py, as this has been deprecated and will
   cause self._initialize to fail

@StewartDouglas StewartDouglas force-pushed the forward_initialize_args branch from c131955 to 6f2e167 Sep 14, 2015

@StewartDouglas

This comment has been minimized.

Contributor

StewartDouglas commented Sep 14, 2015

My Qexec tests failures were due to local configuration issues, and are now passing. Just rebased. @twiecki can you merge if Jenkins passes.

@twiecki

This comment has been minimized.

Contributor

twiecki commented Sep 14, 2015

@StewartDouglas Sure, can you let me know once it does?

@StewartDouglas

This comment has been minimized.

Contributor

StewartDouglas commented Sep 14, 2015

@twiecki sure (and by Jenkins I meant Travis...)

@StewartDouglas StewartDouglas merged commit 6f2e167 into master Sep 14, 2015

2 checks passed

Scrutinizer No new issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@StewartDouglas StewartDouglas deleted the forward_initialize_args branch Sep 14, 2015

@StewartDouglas

This comment has been minimized.

Contributor

StewartDouglas commented Sep 14, 2015

Merged.

@twiecki

This comment has been minimized.

Contributor

twiecki commented Sep 15, 2015

👍

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