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: Make datasets have subclass relationships #924

Merged
merged 5 commits into from Dec 29, 2015

Conversation

Projects
None yet
2 participants
@llllllllll
Member

llllllllll commented Dec 18, 2015

Needed for the EarningsCalendar work we are doing

@llllllllll llllllllll added the Pipeline label Dec 18, 2015

@llllllllll llllllllll force-pushed the dataset-subclassing branch 2 times, most recently from 5d15b97 to a3fecd6 Dec 18, 2015

@@ -119,5 +140,5 @@ def __repr__(self):
return '<DataSet: %r>' % self.__name__


class DataSet(with_metaclass(DataSetMeta)):
class DataSet(with_metaclass(DataSetMeta, object)):

This comment has been minimized.

@ssanderson

ssanderson Dec 28, 2015

Member

Was this previously an old-style class in PY2?

This comment has been minimized.

@llllllllll

llllllllll Dec 28, 2015

Member

yeah, I believe calling type('name', (), {}) creates an old style class. At least it excludes object from the __mro__.

This comment has been minimized.

@ssanderson
return newtype

@property
def columns(self):
return self._columns
return frozenset(

This comment has been minimized.

@ssanderson

ssanderson Dec 28, 2015

Member

Should this be a lazyval now that it's doing semi-nontrivial work?

This comment has been minimized.

@llllllllll

llllllllll Dec 28, 2015

Member

The cost to compute this is still trivial. Idk if we should jump to caching just yet.

newtype = type.__new__(mcls, name, bases, dict_)
_columns = []
newtype = super(DataSetMeta, mcls).__new__(mcls, name, bases, dict_)
column_names = set().union(

This comment has been minimized.

@ssanderson

ssanderson Dec 28, 2015

Member

Might be worth adding a comment here like # Get columns from any parent classes.

bound_column = maybe_column.bind(newtype, maybe_colname)
setattr(newtype, maybe_colname, bound_column)
_columns.append(bound_column)
bound_column_descr = maybe_column.bind(maybe_colname)

This comment has been minimized.

@ssanderson

ssanderson Dec 28, 2015

Member

Might be worth commenting here something like # Add columns from the newly-created class..

sub_dataset_map = {
column.name: column for column in SubDataSet.columns
}
self.assertEqual(

This comment has been minimized.

@ssanderson

ssanderson Dec 28, 2015

Member

I was confused by this assertion when reading. I think it'd be clearer to assert that SomeDataSet.columns is equal to SubDataSet.columns, which I think it both equivalent and closer to the intent of what's being tested here.

This comment has been minimized.

@llllllllll

llllllllll Dec 28, 2015

Member

Sounds good, this is what I am trying to test.

@@ -28,6 +28,10 @@ class SomeDataSet(DataSet):
buzz = Column(float64_dtype)


class SubDataSet(SomeDataSet):

This comment has been minimized.

@ssanderson

ssanderson Dec 28, 2015

Member

Can we add a test case for adding a new column in a subdataset?

@ssanderson

This comment has been minimized.

Member

ssanderson commented Dec 28, 2015

By and large LGTM. Had a couple requests for comments and one request for a testcase.

@llllllllll

This comment has been minimized.

Member

llllllllll commented Dec 29, 2015

added the test and comments. merging on pass

llllllllll added a commit that referenced this pull request Dec 29, 2015

Merge pull request #924 from quantopian/dataset-subclassing
ENH: Make datasets have subclass relationships

@llllllllll llllllllll merged commit 7a6ba4f into master Dec 29, 2015

2 checks passed

Scrutinizer 13 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@llllllllll llllllllll deleted the dataset-subclassing branch Dec 29, 2015

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