-
Notifications
You must be signed in to change notification settings - Fork 6
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
Imageruler revamp #30
Conversation
* Redo package * Fix version number v0.1.0 * Formatting * Docstring darglint * Update imports * Formatting --------- Co-authored-by: Martin Schubert <[email protected]>
Add py.typed
Switch to tometrics algorithm
Reference design readme
Add docs deps
Add TO designs and bump version
Add skimage docs dep
Fix authorship in docs
Notebook for ignore schemes
Nice work! Is there any way to mark the moved / renamed files as such? This would help condense the diffs. |
Unfortunately, this may be difficult to do at this point, but I can list the files which have not changed substantially.
|
|
||
|
||
# ------------------------------------------------------------------------------ | ||
# Array-manipulating functions backed by `cv2`. |
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.
Since we are doing a giant reorg, can we move some of the functions below this line into their own modules (e.g. morphology.py and/or array_utils.py)?
This PR does not contain the function for computing the minimum lengthscale of void regions and the function for computing the overall minimum lengthscale. In the current main branch, the two functions are |
Yes, this was a judgement call---to eliminate redundant functionality, in service of a simpler API for which consistency and correctness is easier to ensure. The overall minimum length scale can be computed by |
Yes, I understand these two functions can be trivially implemented in this way, and I do not insist on adding this trivial wrapper for void or not, but I prefer not to add the trivial wrapper for overall minimum lengthscale as
I think a new function with different implementation is not something that should be avoided if this different implementation has advantage. |
My philosophy here is that the additional code and complexity constitutes a disadvantage, and we have to balance the pros against the cons. I think we may have an opportunity to discuss on Thursday. In any case, with regards to performance there are some improvements with the new code. Here is a benchmarking snippet: def separated_circles(separation_distance: int) -> onp.ndarray:
left_circle = imageruler.get_kernel(80)
right_circle = imageruler.get_kernel(60)
right_circle = onp.pad(right_circle, ((10, 10), (0, 0)))
circles = onp.concatenate(
[left_circle, onp.zeros((80, separation_distance)), right_circle],
axis=1,
)
circles = onp.pad(circles, ((10, 10), (10, 10))).astype(bool)
return circles
print("Using `minimum_length`")
%timeit imageruler.minimum_length(separated_circles(1))
%timeit imageruler.minimum_length(separated_circles(5))
%timeit imageruler.minimum_length(separated_circles(10))
%timeit imageruler.minimum_length(separated_circles(20))
print("Using `minimum_length_solid_void`")
%timeit imageruler.minimum_length_solid_void(separated_circles(1))
%timeit imageruler.minimum_length_solid_void(separated_circles(5))
%timeit imageruler.minimum_length_solid_void(separated_circles(10))
%timeit imageruler.minimum_length_solid_void(separated_circles(20))
print("")
for dim in [1, 5, 10, 20]:
print(f"Separation distance = {dim}")
print(f" Absolute minimum length scale: {imageruler.minimum_length(separated_circles(dim))}")
print(f" Minimum (solid, void): {imageruler.minimum_length_solid_void(separated_circles(dim))}") With the existing implementation (using a colab CPU instance), this prints
As you said, there is some performance benefit for
So, there is a more than 10x performance improvement with the new implementation. |
Nice test and great improvement! The new implementation indeed runs much faster than the old implementation, but the advantage of the |
Yes, I agree with this. |
If performance is not too critical, then the extra code complexity of adding the open–close algorithm might not be worth it for a factor-of-two improvement, at least at the current scales where we expect to apply this code. So, the simplicity of leaving it out might be better. However, if we leave out this optimization, it would be worth adding a comment to the code mentioning this possibility if we want to improve performance in the future. |
I just talked with Steven and now agree with removing the |
Improve docstring and variable naming
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.
I noticed that the three notebooks (to_designs.ipynb
, simple_shapes.ipynb
, and advanced.ipynb
) do not include the outputs from actually running them. Perhaps this was intentional?
Fix module docstring
Correct, to view the output one should look at the docs page, which you can preview here: https://mfschubert.github.io/imageruler/readme.html Docs are automatically generated when there is a push to main branch, and notebook outputs being stripped is actually enforced by one of the pre-commit rules. This way, we avoid having large (code) files as part of the repo. |
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.
Typos.
Fix some typos
Add comment about performance
As discussed in #25 and agreed on during discussion on 3/21, it would be beneficial to simplify the imageruler API and extend testing, so that e.g. automated usage becomes viable. This PR is the follow-up from that discussion.
The changes in this PR include,
With these changes, #25, #24, #23, and #22 can be considered resolved.
For #22 specifically, I created a notebook which shows how the new ignore scheme addresses the issue.
@oskooi @stevengj @mawc2019 @ianwilliamson