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

Refactor sim_params #352

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@twiecki
Contributor

twiecki commented Jun 18, 2014

I stumbled over some problems when trying to change daily events to get the dt of closing day, rather than midnight. I then saw that the code is a little complex and has identical settings in different places that can get out of sync so this is aimed at untangling this code a bit.

@ehebert

This comment has been minimized.

Member

ehebert commented Jun 20, 2014

@twiecki did you run this against the internal test suite, some of the settings for minute/minute sessions may introduce some wrinkles here.

@ehebert

This comment has been minimized.

Member

ehebert commented Jun 20, 2014

@twiecki if you have a chance could you update the commit message with an example of how an algorithm would use the standardized init based SimulationParameters?
The create_simulation_parameters in the unit tests hides some of the details of what a 'normal' usage would look like, at least to my eyes.

(Also, do we need to update the examples to use the new standardized form?)

@coveralls

This comment has been minimized.

coveralls commented Jun 21, 2014

Coverage Status

Coverage increased (+2.86%) when pulling 166947e on refactor_sim_factory into 1b571a5 on master.

twiecki added some commits Jun 18, 2014

MAINT: One way to set sim_params and data_frequency.
There were sevaral places you could supply sim_params
in TradingAlgorithm (__init__, run). This got confusing
as its not clear who updated what and which one was the
correct one to use at each time.

Then there were to ways to define data_frequency, one in
__init__() and one in the sim_params which also added code
complexity.

This refactor makes it explicit that sim_params are to be
passed to __init__() only. Moreover, data_frequency is
only stored in sim_params. For backwards compatibility,
it can still be supplied separately but will link to
the one in sim_params.

For example, you could create new sim params via:

sim_params = create_simulation_parameters(data_frequency='minute')
algo = MyAlgo(sim_params)
algo.run(data)

In addition, perf_tracker only gets initialized in one place:
_create_generator() which should also make the various ways
of running an algorithm more deterministic.

This also fixes a bug with SimulationParameters where
you could not change the period_start. Unfortunately, the
current implementation still requieres an implicit call to
update the internal variables.
@twiecki

This comment has been minimized.

Contributor

twiecki commented Jun 23, 2014

@ehebert OK, I think I did the refactorings. FWIW, I never was a fan of the sim_params to begin with. So a totally different train of thought would revolve around merging SimParameters into TradingAlgorithm. In any case though, there should be only one place for the settings. So I still think we should merge this as it reduces complexity and unexpected behavior and keeps backwards compatibility.

The user-impact should be low in any case as the new CLI hides direct class instantiation.

@twiecki

This comment has been minimized.

Contributor

twiecki commented Jun 23, 2014

I also ran this against the internal test-suite and it passes (after some backwards compat changes already included).

@twiecki

This comment has been minimized.

Contributor

twiecki commented Jun 25, 2014

@ehebert I think this is ready for another round of review.

@ehebert

This comment has been minimized.

Member

ehebert commented Jun 26, 2014

@twiecki I could be reading the timestamps/commitishes incorrectly, but I am not seeing any new/updated commits.

@twiecki

This comment has been minimized.

Contributor

twiecki commented Jun 27, 2014

Oops, forgot to push. Should be updated now.

@coveralls

This comment has been minimized.

coveralls commented Jun 27, 2014

Coverage Status

Coverage decreased (-0.12%) when pulling 295a614 on refactor_sim_factory into 4c9cf13 on master.

@ehebert

This comment has been minimized.

Member

ehebert commented on 295a614 Jun 30, 2014

👍 on removal of the cruft here.

It was a good idea, but we ended up settling on more static values in risk, for now.

@ehebert

This comment has been minimized.

Member

ehebert commented Jun 30, 2014

Thanks the example usage, which is great in its simplicity, helped the reading immensely. /signoff

@twiecki twiecki closed this Jun 30, 2014

@twiecki

This comment has been minimized.

Contributor

twiecki commented Jun 30, 2014

Merged with f8e84ec and 10885e1.

@freddiev4 freddiev4 deleted the refactor_sim_factory branch Sep 6, 2018

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