-
-
Notifications
You must be signed in to change notification settings - Fork 331
document dtype extension #3157
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
base: main
Are you sure you want to change the base?
document dtype extension #3157
Conversation
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.
Nice, this is a great improvment. I left some comments and suggested improvements. The other thing I'd wish for is to the format the lines to to 80 characters long. In general I like 100 line length, but when rendered on the docs page you as it currently stands you have to horizontally scroll to read the example code.
examples/custom_dtype.py
Outdated
|
||
class Int2(ZDType[int2_dtype_cls, int2_scalar_cls]): | ||
""" | ||
This class provides a Zarr compatibility layer around the int2 data type and the int2 |
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.
Is there a nice link explaining the difference between these? I think I've inferred it but would be nice to make it explicit.
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.
no I don't actually think there is a nice link that explains the data type / scalar type difference. The numpy docs should explain this, but they don't. I can add something to our docs.
examples/custom_dtype.py
Outdated
|
||
def to_json_scalar(self, data: object, *, zarr_format: ZarrFormat) -> int: | ||
"""Convert a python object to a scalar.""" | ||
return int(data) |
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.
Can this be more specific to the example? e.g. explain something to the effect of "needs to be int
to be compatible with json." and mention int2
somewhere.
I think the content is basically done -- I added docstrings to basically every new class and method (please let me know if i missed anything). I am however currently stymied on some sphinx build issues, so if anyone is proficient in that kind of debugging I would appreciate help. |
Build issue is somehow related to this i think: |
Broken in a new way! Crazy that that fixed that |
all the sphinx issues are sorted, the only failing check is coverage, and I'm not sure I believe the claims that these changes reduced test coverage by 20%. |
This PR adds a working example of custom dtype creation and registration. because it's a lot of code, I put this in a new top-level directory called
examples
, which contains the executable python filedtype_example.py
. This file uses PEP-723 metadata to declare aml_dtypes
dependency, and it uses a local zarr install, which means it can be tested properly against local changes.I also expanded the current dtype docs in the user guide to include content about the data type resolution process.
TODO:
docs/user-guide/*.rst
changes/