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: Adds tick_size and renames futures multiplier #941

Merged
merged 6 commits into from Jan 25, 2016

Conversation

Projects
None yet
3 participants
@jfkirk
Contributor

jfkirk commented Jan 5, 2016

This PR adds assets db version downgrade management in a new module: asset_db_migrations.py

This PR also has three effects:
'tick_size' is added to Future objects as an informational field. It is not read outside of the object.
The field that was called 'contract_multiplier' is renamed to 'multiplier'
The ASSETS_DB_VERSION is now v1.

@jfkirk jfkirk force-pushed the futures-tick-size branch 2 times, most recently from 51792d5 to ce19a9f Jan 6, 2016

self.assertTrue('futures_contracts' in metadata.tables)
self.assertTrue('version_info' in metadata.tables)
self.assertFalse('tick_size' in
metadata.tables['futures_contracts'].columns)

This comment has been minimized.

@StewartDouglas

StewartDouglas Jan 7, 2016

Contributor

How about including self.assertFalse('multiplier' in metadata.tables['futures_contracts'].columns) here, to make sure this column has also been successfully deleted?

@StewartDouglas

This comment has been minimized.

Contributor

StewartDouglas commented Jan 7, 2016

I'm assuming that alembic also takes care on PRIMARY keys? i.e. we wouldn't need any additional logic added if we wished to rename a PRIMARY key?

@jfkirk

This comment has been minimized.

Contributor

jfkirk commented Jan 7, 2016

Alembic properly migrates the primary keys by making the corresponding columns primary keys on the temp table when executing a batch alteration. You can add/remove a column's primary key status through an alter_column call.

op.drop_index('ix_futures_contracts_root_symbol')
op.drop_index('ix_futures_contracts_symbol')
# Execute batch op to allow column modification in SQLLite

This comment has been minimized.

@StewartDouglas

StewartDouglas Jan 7, 2016

Contributor

It's just SQLite. Sorry to be pedantic 😄

@StewartDouglas

This comment has been minimized.

Contributor

StewartDouglas commented Jan 7, 2016

Apart from the minor points I raised this LGTM

@@ -315,7 +321,8 @@ def test_to_and_from_dict(self):
self.assertTrue('notice_date' in dictd)
self.assertTrue('expiration_date' in dictd)
self.assertTrue('auto_close_date' in dictd)
self.assertTrue('contract_multiplier' in dictd)
self.assertTrue('tick_size' in dictd)

This comment has been minimized.

@ssanderson

ssanderson Jan 7, 2016

Member

Unrelated to this change, but why does this test not assert anything about the contents of the dict here? It would be the same amount of work for this to do:

self.assertEqual(dictd['root_symbol'], <expected symbol>)
...

or better yet:

self.assertEqual(self.future.to_dict(), {key: getattr(self.future, key) for key in dir(Future))

which will never need to be updated if we add a new attributr.

This comment has been minimized.

@jfkirk

jfkirk Jan 7, 2016

Contributor

I like that - will update

@StewartDouglas

This comment has been minimized.

Contributor

StewartDouglas commented Jan 11, 2016

It's dawned on me that we should be defining a tick_size and multiplier columns on the root_symbols table. This is because when we see a new futures contract coming into existence, we will need to lookup these values somewhere, which is something we don't currently have in place.

@jfkirk jfkirk force-pushed the futures-tick-size branch from ce19a9f to 0270d92 Jan 22, 2016

@ssanderson ssanderson force-pushed the master branch from 110798b to 0dac2e0 Jan 22, 2016

@jfkirk jfkirk force-pushed the futures-tick-size branch from 0270d92 to c9c2e3e Jan 22, 2016

jfkirk added some commits Jan 22, 2016

@StewartDouglas

This comment has been minimized.

Contributor

StewartDouglas commented Jan 22, 2016

LGTM 👍

jfkirk added a commit that referenced this pull request Jan 25, 2016

Merge pull request #941 from quantopian/futures-tick-size
ENH: Adds tick_size and renames futures multiplier

@jfkirk jfkirk merged commit 2803802 into master Jan 25, 2016

1 check passed

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

@jfkirk jfkirk deleted the futures-tick-size branch Jan 25, 2016

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