-
Notifications
You must be signed in to change notification settings - Fork 3
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
Update pytest and rm pytest-pep8 dependency. #40
Conversation
pytest-pep8 is no longer supported, and pytest<6 is likely the source of the issues with coveralls.
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.
Okay this should be ready for review now. This PR includes a couple maintenance items to get the CI system working again and fixing some of the more disruptive problems related to changes in underlying dependencies (e.g. numpy). It is by no means comprehensive and there's still more maintenance work to do, but this is a minimal set of patches to make CI green again.
See inline comments for details (or ignore them - I mostly included them only to explain changes in more detail).
@@ -58,9 +58,9 @@ def slice_image(X, reshape_size, overlap=0): | |||
L_y = reshape_size[1] # y length of each slice | |||
|
|||
# number of slices along x axis | |||
n_x = np.int(np.ceil((image_size_x - 2 * L_x + overlap) / (L_x - overlap)) + 2) | |||
n_x = np.int_(np.ceil((image_size_x - 2 * L_x + overlap) / (L_x - overlap)) + 2) |
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.
NumPy 1.24 removed the np.int
, np.int_
is the preferred way to do this and behaves the same; though whether this really needs to be a platform integer is up for debate. If the only intent here is to get an integer scalar out then its even better to replace np.int_
with just int
.
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.
In that case, I think changing to int
would be fine.
np.ceil((image_size_x - 2 * L_x + overlap) / (L_x - overlap)) + 2) | ||
|
||
new_batch_size = X.shape[0] * n_y * n_x # number of images in output | ||
|
||
new_X_shape = (new_batch_size, L_y, L_x, X.shape[3]) | ||
new_X = np.zeros(new_X_shape, dtype=K.floatx()) | ||
|
||
new_y = [None] * new_batch_size | ||
new_y = np.array([None] * new_batch_size, dtype=object) |
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 explicitly creates an object array, which is the only way in numpy to create a "ragged" array; i.e. one where the shape may change along a given dimension.
Note that object arrays are often not desirable, and there may be a better data structure for this particular object - let's earmark that for future discussion.
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 have been wanting to update this data structure from a ragged list of lists for awhile, but didn't have the time to invest in making the change. I would love to hear your thoughts on better solutions!
While looking at #41 I noticed new failures in CI from networkx - this is due to the removal of the I'm personally responsible for that change in NX, so at least you know where to lodge complaints 🙃 |
pytest-pep8 is no longer supported, and pytest<6 is likely the source of the issues with coveralls.
I will re-add some form of linting to CI to replace pytest-pep8 in the near future (this applies to all the
deepcell-*
libraries). See also vanvalenlab/deepcell-toolbox#128.