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

Support for setting parameters externally #456

Closed
wants to merge 7 commits into from
Closed

Conversation

twiecki
Copy link
Contributor

@twiecki twiecki commented Dec 26, 2014

The reason for this change is that it was impossible to
use a TradingAlgorithm that passes in an initialize function
in a parameter optimization. This issue is discussed in #455.

It seems easy enough to just forward any left-over _args
and *_kwargs to the user-defined initialize function. As these
are only passed when they are defined we do not break backwards
compatibility with this.

@twiecki
Copy link
Contributor Author

twiecki commented Dec 29, 2014

Depends on #459.

@twiecki twiecki force-pushed the param_set branch 2 times, most recently from b98c744 to 4c19066 Compare December 29, 2014 13:52
@ssanderson
Copy link
Contributor

I'm +1 on making it easier to sweep a parameter space, but I think I'm -1 on this as an API.

Forwarding parameters from __init__ to initialize makes our already spaghetti API for setting up a TradingAlgorithm even more spaghetti. With this change you'd have to think about and understand the differences between:

  • The args you're passing to __init__ (which includes sim_params, itself a bundle of arguments).
  • The subset of those args that are forwarded to initialize and/or _before_trading_start.
  • The args you're passing to run, some of which may override the parameters described above.

I'm also generally -1 on the use of *args and **kwargs for public-facing APIs, because they obscure the natural documentation of function signatures and they often mask subtle errors (e.g. typos in keyword parameter names).

@twiecki
Copy link
Contributor Author

twiecki commented Dec 29, 2014

I agree with all your points.

One other easy way would be a dict you pass specifically to __init__, e.g. initialize_params={'param1': 3}. Or we finally untangle the mess that is initialize but I need parameter sweeps very soon.

@ssanderson
Copy link
Contributor

Or we finally untangle the mess that is initialize but I need parameter sweeps very soon.

I think this isn't actually that much work to do. We mostly just need to decide what we actually want the model to be. Part of that change likely also involves killing the subclass-and-override API, which I think has proven much harder to support in a clean way than the algoscript and function-passing APIs.

@twiecki
Copy link
Contributor Author

twiecki commented Dec 29, 2014

I think this isn't actually that much work to do.

I'm a bit more pessimistic. Unless you have the resources to spearhead that effort? If not, would you be fine with the interim solution of an initialize_params kwarg?

We mostly just need to decide what we actually want the model to be. Part of that change likely also involves killing the subclass-and-override API, which I think has proven much harder to support in a clean way than the algoscript and function-passing APIs.

Sounds like something we should discuss in a hangout if we want to start tackling that.

@twiecki twiecki changed the title ENH: Forward *args and **kwargs from __init__ to initialize. ENH: Add initialize_params keyword arg. Dec 29, 2014
@twiecki
Copy link
Contributor Author

twiecki commented Dec 30, 2014

Turns out we were already forwarding *args and **kwargs when using the inheritance based patterns and a lot of code depends on it (e.g. https://github.com/quantopian/zipline/blob/master/zipline/test_algorithms.py#L100). In that light, forwarding in the same way for the algo_script interface would increase consistency. I still agree with your points above but this would require a larger refactoring of many parts of the code base. Not saying we shouldn't do it but we'd need to do that soon.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) when pulling 0a55b3b on param_set into 994f7ce on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) when pulling 0a55b3b on param_set into 994f7ce on master.

@femtotrader
Copy link

Passing a dict for backtesting parameters is an interesting solution. But you should also have a look at Bunch objects https://pypi.python.org/pypi/bunch/1.0.1 they are very convenient to access parameters in an "object" manner.

@twiecki
Copy link
Contributor Author

twiecki commented Dec 30, 2014

Thanks for the pointer @femtotrader. If those can be unpacked using **bunch_obj you should be able to just pass that in already.

@twiecki twiecki changed the title ENH: Add initialize_params keyword arg. Support for setting parameters externally Dec 30, 2014
@twiecki
Copy link
Contributor Author

twiecki commented Jan 7, 2015

ping @ssanderson

@ssanderson
Copy link
Contributor

@twiecki sorry, I lost track of this one.

I won't have bandwidth to work on an implementation of this until mid-March I think. In the shorter term I could probably spend a day or two hashing out what I think is the right design (with input from you and @ehebert, @llllllllll and others of course).

I'm really hesitant to use the "we need this soon" argument as a reason to change the API in a way that we agree makes it worse, since that's trading a one-time benefit for a compounding cost. What's the timescale on which you think you need parameter optimization?

@ssanderson
Copy link
Contributor

If we truly do need to do something in the short term, I'd prefer it be something that's explicitly marked as provisional and that's easy to remove in the future. That's probably easier to do with your explicit initialize_kwargs proposal, so I'd probably lean toward that if push came to shove.

@twiecki
Copy link
Contributor Author

twiecki commented Jan 7, 2015

@ssanderson We just had our kick-off meeting on that so very soon.

Yeah, I think the initialize_kwargs is pretty easy to remove afterwards. Alternatively we could also work on this branch but I'm afraid that it creates some good friction elsewhere in the process.

@twiecki
Copy link
Contributor Author

twiecki commented Feb 12, 2015

@ssanderson Any update? I don't see us refactoring this soon and I really need this functionality.

@c0indev3l
Copy link

Hello,

I'm also looking for such a feature.
It will help to have walk forward optimization for example.

I wonder if passing an additional parameter to algo
parameters={"param1": value1, "param2"; value2}

my_algo = TradingAlgorithm(
initialize=initialize,
handle_data=handle_data,

parameters=parameters

)

Parameters could be available through
context.parameters['param1']

It shouldn't be so hard and it shouldn't break anything.

Kind regards

2015-02-12 14:31 GMT+01:00 Thomas Wiecki notifications@github.com:

@ssanderson https://github.com/ssanderson Any update? I don't see us
refactoring this soon and I really need this functionality.


Reply to this email directly or view it on GitHub
#456 (comment).

@GiveMeMoreLoansZeroInterest GiveMeMoreLoansZeroInterest removed their assignment Feb 12, 2015
The reason for this change is that it was impossible to
use a TradingAlgorithm that passes in an initialize function
in a parameter optimization. This issue is discussed in #455.

It seems easy enough to just forward any left-over *args
and **kwargs to the user-defined initialize function. As these
are only passed when they are defined we do not break backwards
compatibility with this.
@twiecki
Copy link
Contributor Author

twiecki commented Feb 13, 2015

@c0indev3l That's exactly what this PR would allow. It's pretty simple to do actually. I think I'll just merge this as it's critical functionality and easy to remove later when we actually do refactor this.

@warren-oneill
Copy link
Contributor

@twiecki @ssanderson Any word on this? Trying to implement optimization techniques in Zipline is very ugly without this PR and it seems like a feature that quite a few people want. Having a backtester without a clean way of optimizing is a bit non-nonsensical imo.

@twiecki
Copy link
Contributor Author

twiecki commented Apr 10, 2015

Agree with @warren-oneill. I'm also not convinced anymore that .run() should call initialize(). The use case of calling .run() multiple times with different data would not be supported anymore. A larger refactoring realistically is not going to happen any time soon so we should merge this and unlock a huge number of use cases with a fairly simple code change.

@twiecki
Copy link
Contributor Author

twiecki commented Apr 23, 2015

I have a much simpler idea for implementing this:

Instead of changing when initialize gets called or passing through parameters from __init__, we could just add a call_initialize kwarg to __init__ that causes the user initialize() not get called. Instead, the user can just call it himself before a call to .run().

@warren-oneill
Copy link
Contributor

Makes sense but one thing that hasn't been tackled, as far as I'm aware, is a rolling optimization. Or is it possible with this setup?
I think as well as an initialize one would need, e.g, optimize_daily() which would pick the parameters that preformed the best over the last x number of days and then overwrites the parameters in the algo.
Of course this can also come in as stage 2. What do you think?

@twiecki
Copy link
Contributor Author

twiecki commented Apr 27, 2015

That's true that this is not addressed. It's a bit more tricky though as the algorithm state would have to saved (pickled) and different versions be run over the same time-period.

@twiecki
Copy link
Contributor Author

twiecki commented Jun 11, 2015

Closing in favor of #600. We should port the example from here however.

@twiecki twiecki closed this Jun 11, 2015
@freddiev4 freddiev4 deleted the param_set branch September 7, 2018 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants