Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
WIP: generator accepts NdArray attribute #575
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
WIP: generator accepts NdArray attribute #575
Changes from all commits
a7907cf
def940c
b6e363a
cc3423b
a6fa453
4cb691c
16bd32f
16a934b
dd85896
4731eb3
148a03b
aca7510
5d95063
0cb0987
45b4ce0
0193898
d082966
a4d8069
2ba663f
6e58a57
0449b44
92d9eac
54c4355
e4afbba
bcf7e3b
5702eb1
cc86f95
3ab5c8f
e1cd712
5886cb5
f0ec23f
605e374
672c54a
d98ee2d
c4f1279
f355196
b8f7784
d5c4184
4229358
e0f1a37
b729235
33ebd58
c02d5f8
a7e73a9
7599c13
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 we need some docstring for the class. I understand that many other classes in this file don't have that, but that's a historical mistake, let's add docstring as much as possible for future code.
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.
Agree
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.
do we have some tests to validate a JSON spec with this schema, hopefully, both success and failure cases?
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.
Added a test against invalid shape.
Do you think we can rely on numpy to validate dtype?
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.
it'd be great if schema can check the possible dtype values. Since this is done without actually running the code, while numpy validation happens late at run time.
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.
sounds good.
I've added more tests against dtype and shape
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.
the validation looks awesome, and thanks for the tests