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
Move Domain
and Dim
Into Pure Python (2/3)
#1327
Conversation
67f184c
to
a59b084
Compare
a59b084
to
2f81560
Compare
2f81560
to
cd0f25e
Compare
f1bb555
to
c5b5a7a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's talk through the design a bit more here. _lt_obj
arguments don't quite seem right. __init__
methods should take arguments that are used to construct the class, and those _lt_obj
arguments could be moved into a staticmethod factor constructor which forwards to __init__
. But I want to make sure I understand why those are necessary (similar to the next point)
c5b5a7a
to
4f26b9c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with some minor nits. Thanks
|
||
super().__init__(self._ctx, name, dim_datatype, domain_array, tile_size_array) | ||
|
||
if filters is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see if we can move this to the constructor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is easier to just leave it as is since the create function does not take in a filter list argument.
https://github.com/TileDB-Inc/TileDB/blob/dev/tiledb/sm/cpp_api/dimension.h#L430-L478
4f26b9c
to
d84a686
Compare
* This should have been reverted before merging #1327, but I forgot to modify
* This should have been reverted before merging #1327, but I forgot to modify
* This should have been reverted before merging #1327, but I forgot to modify
* This should have been reverted before merging #1327, but I forgot to modify
This is a subset of #1188. Includes changes from #1326.