-
Notifications
You must be signed in to change notification settings - Fork 20
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
[InMemoryDataset redesign] fill_hyperspace() #371
Conversation
# Note: Py_ssize_t is always the same as np.intp | ||
assert np.intp().nbytes == _py_ssize_t_nbytes | ||
|
||
if cython.compiled: # pragma: nocover |
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.
One of the many things I love about cython pure python mode: you can just disable cythonization and run coverage on it!
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.
As an only occasional cython user, this seems incredibly useful! Lately I've been discovering more reasons to use cython for compiled extensions...add this to the list. It seems like the only downside is that for cython-only code you can't run coverage on it - but that's still a big improvement, since the rest of the module is covered!
d509d16
to
baadb1c
Compare
@@ -0,0 +1 @@ | |||
hyperspace.py |
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.
This is a symlink hack to make meson compile a pure-python cython module
# To generate HTML compilation reports: | ||
# (First double check that flags below match those above) | ||
# | ||
# cythonize -3 -a -X infer_types=True -X initializedcheck=False -X boundscheck=False -X wraparound=False -X cdivision=True versioned_hdf5/{hyperspace,subchunk_map}.py |
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.
This line is incorrect until the next PR that will add the same flags to subchunk_map.py
8135291
to
86a86a0
Compare
# instead of its compiled form. This fails in CI, as Cython is not a runtime dependency, | ||
# and regardless would cause a lot of type-checking normally performed by Cython to be | ||
# skipped so it is to be avoided. | ||
from . import hyperspace # noqa: F401 |
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.
This will later be replaced by a proper import of functionality
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.
Understood. Maybe the later revision will address this, but
- we can also make
cython
a test dependency if needed, and - I don't think we have any doctests?
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.
hyperspace.py has two doctests.
Making cython a test dependency would be a bad idea because, while it would fix the CI failure, it would cause ALL tests to use the pure-python version of hyperspace.py, which is much more lenient in terms of typing than the cythonized version, thus potentially hiding breaking bugs completely until someone manually tries using it.
86a86a0
to
b5442f3
Compare
b5442f3
to
7cd6427
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.
This first addition looks great. Thank you especially for the very thorough documentation, because _hyperrectangles_between
would be quite hard to follow without it! 🚀
# Note: Py_ssize_t is always the same as np.intp | ||
assert np.intp().nbytes == _py_ssize_t_nbytes | ||
|
||
if cython.compiled: # pragma: nocover |
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.
As an only occasional cython user, this seems incredibly useful! Lately I've been discovering more reasons to use cython for compiled extensions...add this to the list. It seems like the only downside is that for cython-only code you can't run coverage on it - but that's still a big improvement, since the rest of the module is covered!
assert np.intp().nbytes == _py_ssize_t_nbytes | ||
|
||
if cython.compiled: # pragma: nocover | ||
from cython.cimports.cpython import array # type: ignore |
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.
Hmm, I guess types are are not supplied by cython?
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 don't follow, what types?
# instead of its compiled form. This fails in CI, as Cython is not a runtime dependency, | ||
# and regardless would cause a lot of type-checking normally performed by Cython to be | ||
# skipped so it is to be avoided. | ||
from . import hyperspace # noqa: F401 |
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.
Understood. Maybe the later revision will address this, but
- we can also make
cython
a test dependency if needed, and - I don't think we have any doctests?
* Revert #371 * Salvage changes
This is a partial PR that breaks down #370 into digestible chunks - see #370 (comment)
This PR adds a single new utility function,
fill_hyperspace
, that for the time being remains unused and will be later used by #370.