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

ENH: Change default commission to .001 #1946

Merged
merged 1 commit into from Oct 4, 2017

Conversation

Projects
None yet
5 participants
@srklonaris
Contributor

srklonaris commented Sep 14, 2017

Change default commission to .001 to be consistent with the contest.

@srklonaris srklonaris requested a review from dmichalowicz Sep 14, 2017

@jbredeche

This comment has been minimized.

Member

jbredeche commented Sep 15, 2017

what about DEFAULT_MINIMUM_COST_PER_EQUITY_TRADE? do we still want a minimum of $1/trade?

@srklonaris srklonaris force-pushed the default-commissions branch 4 times, most recently from da40c86 to 5b3e2de Sep 19, 2017

@srklonaris

This comment has been minimized.

Contributor

srklonaris commented Sep 25, 2017

After discussion with Josh, I also updated DEFAULT_MINIMUM_COST_PER_EQUITY_TRADE and DEFAULT_MINIMUM_COST_PER_FUTURE_TRADE to both be 0.0.

I had to update some of the tests too. Because the issue here: https://github.com/quantopian/zipline/blob/master/tests/resources/rebuild_example_data#L105 I can't rebuild the example data, so I had to manually set the commisions in each example to the old commision values so that the results would be the same. This should be a temporary fix until we can rebuld the example data again.

@dmichalowicz do you mind taking a look again?

self.assertEqual(results.port_value.iloc[0], 10000)
self.assertAlmostEqual(results.port_value.iloc[1],
10000 + 780 - 392 - 1)
10000 + 780 - 392 - 0,
places=2)

This comment has been minimized.

@srklonaris

srklonaris Sep 25, 2017

Contributor

I was getting this failure '10387.99899997449 != 10388 within 7 places' without adding places=2 I'm not sure if there is a better way to do this.

This comment has been minimized.

@dmichalowicz

dmichalowicz Sep 26, 2017

Contributor

I think this is fine.

@coveralls

This comment has been minimized.

coveralls commented Sep 25, 2017

Coverage Status

Coverage increased (+0.008%) to 87.212% when pulling da40c86 on default-commissions into 64af3d3 on master.

@coveralls

This comment has been minimized.

coveralls commented Sep 25, 2017

Coverage Status

Coverage increased (+0.008%) to 87.212% when pulling 5b3e2de on default-commissions into 64af3d3 on master.

self.assertEqual(results.port_value.iloc[0], 10000)
self.assertAlmostEqual(results.port_value.iloc[1],
10000 + 780 - 392 - 1)
10000 + 780 - 392 - 0,
places=2)

This comment has been minimized.

@dmichalowicz

dmichalowicz Sep 26, 2017

Contributor

I think this is fine.

# Explicitly set the commission to the "old" value until we can
# rebuild example data.
# github.com/quantopian/zipline/blob/master/tests/resources/
# rebuild_example_data#L105

This comment has been minimized.

@dmichalowicz

dmichalowicz Sep 26, 2017

Contributor

TLDR: What you have here is good.

It's actually still possible to rebuild the example data (with a couple minor tweaks), I did so here only about a month ago. We now get benchmark data from Google instead of Yahoo, as seen here.

However, it appears that as of only a week or two ago, Google changed the URL from which they are serving their financial data, causing pandas datareader to break. As a result, after checking out your branch I was unable to rebuild the data like I was previously (without having to either (1) make our own pandas-datareader fork, or (2) make a bunch of extra changes to the tests and test dates; neither of which I want or think we need to do).

For more info see the following:

All that being said, I think your solution of keeping the old parameters for now is best. When the pandas datareader issue is fixed though I think this may be worth revisiting.

@richafrank @freddiev4 Is this breakage something we should be worried about anywhere else?

This comment has been minimized.

@richafrank

richafrank Sep 27, 2017

Member

Yea, I think we need to revisit our benchmark downloading, either fixing google, adding back yahoo (via pandas-datareader if it supports it), or both.

This comment has been minimized.

@dmichalowicz
@dmichalowicz

This comment has been minimized.

Contributor

dmichalowicz commented Sep 26, 2017

@srklonaris Looks good to me

@dmichalowicz

This comment has been minimized.

Contributor

dmichalowicz commented Sep 26, 2017

@srklonaris Quick note before you merge, I would rebase this off of master first as it looked like it was branched off a really old commit

@srklonaris srklonaris force-pushed the default-commissions branch from 5b3e2de to 49db008 Sep 26, 2017

@coveralls

This comment has been minimized.

coveralls commented Sep 26, 2017

Coverage Status

Coverage increased (+0.008%) to 87.212% when pulling 49db008 on default-commissions into ec94e5a on master.

@srklonaris srklonaris merged commit 31447d6 into master Oct 4, 2017

2 checks passed

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

@srklonaris srklonaris deleted the default-commissions branch Oct 4, 2017

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