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 auto_close_date support for equities #982

Merged
merged 2 commits into from Feb 23, 2016

Conversation

Projects
None yet
3 participants
@dmichalowicz
Contributor

dmichalowicz commented Feb 4, 2016

  • Adds an auto_close_date field to the Asset object, which is inherited by both equities and futures.
  • Creates an auto_close_date column for equities in the schema of the assets database.
  • Now closes equities and futures immediately prior to before_trading_start, as opposed to at market close.
@dmichalowicz

This comment has been minimized.

Contributor

dmichalowicz commented Feb 4, 2016

@jfkirk Assigning this to you for now, though still a work in progress. Also ping @ssanderson @richafrank

@dmichalowicz

This comment has been minimized.

Contributor

dmichalowicz commented Feb 11, 2016

@jfkirk @ssanderson This is ready for review.

self.panel = pd.Panel({1: pd.DataFrame({
'price': [1, 1, 2, 4], 'volume': [1e9, 1e9, 1e9, 0],
'type': [DATASOURCE_TYPE.TRADE,
DATASOURCE_TYPE.TRADE,
DATASOURCE_TYPE.TRADE,
DATASOURCE_TYPE.CLOSE_POSITION]},
index=self.days)
index=self.days[:4])
})
self.no_close_panel = pd.Panel({1: pd.DataFrame({

This comment has been minimized.

@ssanderson

ssanderson Feb 11, 2016

Member

Why are these now different lengths?

This comment has been minimized.

@dmichalowicz

dmichalowicz Feb 11, 2016

Contributor

As a result of auto-closing in before_trading_start as opposed to in handle_market_close, futures with an auto_close_date of the day after the last day of the simulation will no longer be auto-closed. With that said, I should work on a better test for that than this.

)
# Make all start dates equal.
self.metadata['start_date'] = [self.metadata['start_date'][0]]*3

This comment has been minimized.

@ssanderson

ssanderson Feb 11, 2016

Member

You can just do df[column] = scalar instead of writing a list:

In [1]: df
Out[1]:
           a         b   c
0  -0.232151 -0.601906   5
1   1.795220 -0.535771   5
2   0.752929 -1.103029   5
3  -0.026765 -1.379774   5

In [2]: df.c = 1

In [3]: df
Out[3]:
           a         b  c
0  -0.232151 -0.601906  1
1   1.795220 -0.535771  1
2   0.752929 -1.103029  1
3  -0.026765 -1.379774  1
4   0.064691  1.046385  1
Make sure that after a security gets delisted, our portfolio holds the
correct number of securities and correct amount of cash.
"""
self.env.write_data(equities_df=self.metadata)

This comment has been minimized.

@ssanderson

ssanderson Feb 11, 2016

Member

You're calling write_data in all of these tests with self.metadata. Can we move that logic into the shared setup (it should probably be in a setUpClass for performance)?

algo = TestLiquidationAlgorithm(env=self.env)
algo.run(self.source)
expected_cash = [100000, 99830, 99830, 99860, 99860, 99940, 99940]

This comment has been minimized.

@ssanderson

ssanderson Feb 11, 2016

Member

Can we annotate these changes to make it clearer what's being tested here?

algo = TestLiquidationAlgorithm(env=self.env)
algo.run(self.source)
expected_cash = [100000, 99830, 99830, 99830, 99860, 99860, 99940]

This comment has been minimized.

@ssanderson

ssanderson Feb 11, 2016

Member

same comment as above.

# Drop indices before batch
# This is to prevent index collision when creating the temp table
op.drop_index('ix_equities_fuzzy_symbol')

This comment has been minimized.

@ssanderson

ssanderson Feb 11, 2016

Member

It looks like this is copied from the migration above. @jfkirk do we need to add this to every downgrade? I'd be a bit surprised if that were the case.

This comment has been minimized.

@jfkirk

jfkirk Feb 11, 2016

Contributor

I believe we will need it for every downgrade that uses a batch_alter_table. @dmichalowicz , did you try this downgrade without the index drops?

This comment has been minimized.

@dmichalowicz

dmichalowicz Feb 11, 2016

Contributor

Yes I tried it without the index drops but it failed

@@ -172,6 +172,27 @@ def build_lookup_generic_cases(asset_finder_type):
class AssetTestCase(TestCase):
asset_attrs = ['sid',

This comment has been minimized.

@jfkirk

jfkirk Feb 11, 2016

Contributor

It seems like we may want to add this list to assets.py, rather than here in the tests.

This comment has been minimized.

@ssanderson

ssanderson Feb 11, 2016

Member

Better yet, we should be dynamically generating this list from the types themselves:

In [1]: from zipline.assets import Asset
f
In [2]: from types import GetSetDescriptorType

In [3]: [name for name, value in vars(Asset).items() if isinstance(value, GetSetDescriptorType)]
Out[3]:
['auto_close_date',
 'sid',
 'start_date',
 'end_date',
 'exchange',
 'symbol',
 'first_traded',
 'asset_name']
self.env = TradingEnvironment()
ix = self.env.trading_days.get_loc(dt)
self.metadata = make_rotating_equity_info(

This comment has been minimized.

@ssanderson

ssanderson Feb 11, 2016

Member

I'm inclined to say we should move the metadata construction here into a new test_utils function called make_jagged_equity_info or somesuch.

@@ -2018,3 +2022,99 @@ def test_remove_data(self):
# initially only data for X should be sent and on the last day only
# data for Y should be sent since X is expired
np.testing.assert_array_equal(self.algo.data, expected_lengths)
class TestAssetAutoClose(TestCase):

This comment has been minimized.

@ssanderson

ssanderson Feb 11, 2016

Member

This is just testing auto closes of equities, right?

This comment has been minimized.

@dmichalowicz

dmichalowicz Feb 11, 2016

Contributor

Yes, will change name.

def tearDown(self):
del self.env
def test_delisted_securities(self):

This comment has been minimized.

@ssanderson

ssanderson Feb 11, 2016

Member

Since this is specifically testing equities, I'd call this test_delisted_equities (with similar modifications below).

@@ -56,6 +57,16 @@
'net_value'])
def generate_close_event(sid, date):
# Return a CLOSE_POSITION event

This comment has been minimized.

@ssanderson

ssanderson Feb 11, 2016

Member

Indentation here is off.

'sid': sid,
})
yield event
yield generate_close_event(sid, date)

This comment has been minimized.

@ssanderson

ssanderson Feb 11, 2016

Member

The verb generate here makes me think that generate_close_event is a generator function.

@@ -362,7 +368,7 @@ def maybe_create_close_position_transaction(self, event):
dt=event.dt,
price=price,
commission=0,
order_id=0
order_id=uuid.uuid4().hex,

This comment has been minimized.

@ssanderson

ssanderson Feb 11, 2016

Member

Why did this change?

@@ -947,6 +949,28 @@ def handle_data(self, data):
self.i += 1
class TestLiquidationAlgorithm(TradingAlgorithm):

This comment has been minimized.

@ssanderson

ssanderson Feb 11, 2016

Member

We're generally moving away from the model of subclassing TradingAlgorithm for specific algos. I'd rather write this as:

def initialize(context):
    ...

def handle_data(context, data):
    ...

algo = TradingAlgorithm(initialize=initialize, handle_data=handle_data, ...)
def handle_data(self, data):
if not self.ordered:
self.ordered = True

This comment has been minimized.

@ssanderson

ssanderson Feb 11, 2016

Member

General coding style note: when I have a flag signalling that some operation has been performed, I generally try to set it only after that operation has been performed. Here it's harmless, but it makes you more likely to do the right/expected thing in more complicated setups.

self.assertEqual(algo.cash, expected_cash)
self.assertEqual(algo.num_positions, expected_num_positions)
def test_cancel_open_orders(self):

This comment has been minimized.

@ssanderson

ssanderson Feb 11, 2016

Member

I'm not sure this actually tests what we want. If the orders are just filled normally, there won't be any open orders by the time the algo finishes. At the very least, we should confirm that there are open orders at the end of the day before liquidation, and that they're removed on the subsequent call.

This comment has been minimized.

@dmichalowicz

dmichalowicz Feb 16, 2016

Contributor

My thinking here was that if you place an order for a stock the day before it delists, it will never get filled and under the current circumstances would remain open indefinitely, meaning at the end of the backtest you'd still have an open order for it. This tests that at the end of the backtest this is not the case. I will fix it though to make it clearer.

self.panel = pd.Panel({1: pd.DataFrame({
'price': [1, 1, 2, 4], 'volume': [1e9, 1e9, 1e9, 0],
'price': [1, 1, 2, 4, 8], 'volume': [1e9, 1e9, 1e9, 1e9, 0],

This comment has been minimized.

@dmichalowicz

dmichalowicz Feb 16, 2016

Contributor

I just made this the same length as below for consistency.

@dmichalowicz dmichalowicz force-pushed the liquidation branch from 0239dde to 0275076 Feb 17, 2016

context.num_positions.append(len(context.portfolio.positions))
algo = TradingAlgorithm(
initialize=self.initialize.im_func,

This comment has been minimized.

@dmichalowicz

dmichalowicz Feb 17, 2016

Contributor

Not sure if im_func is the correct way to do this.

Make sure that after an equity gets delisted, our portfolio holds the
correct number of equities and correct amount of cash.
"""
def handle_data(context, data):

This comment has been minimized.

@dmichalowicz

dmichalowicz Feb 17, 2016

Contributor

Each test's handle_data is slightly different; is it worth defining it every test, or should I try to make a shared handle_data like initialize?

@ssanderson ssanderson assigned ssanderson and unassigned jfkirk Feb 18, 2016

@dmichalowicz dmichalowicz changed the title from WIP: Add auto_close_date support for equities to Add auto_close_date support for equities Feb 19, 2016

# Execute batch op to allow column modification in SQLite
with op.batch_alter_table('equities') as batch_op:
# Delete 'auto_close_date'

This comment has been minimized.

@ssanderson

ssanderson Feb 19, 2016

Member

This comment isn't adding any useful information here.

@ssanderson ssanderson force-pushed the liquidation branch from b03d1e4 to 7546c88 Feb 22, 2016

@dmichalowicz dmichalowicz force-pushed the liquidation branch from 7546c88 to 5be63f3 Feb 22, 2016

ssanderson added a commit that referenced this pull request Feb 23, 2016

Merge pull request #982 from quantopian/liquidation
Add auto_close_date support for equities

@ssanderson ssanderson merged commit fa8dd42 into master Feb 23, 2016

@ssanderson ssanderson deleted the liquidation branch Feb 23, 2016

@twiecki twiecki referenced this pull request Mar 7, 2016

Closed

Add LIQUIDATION events #460

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