Apply tiled functionality for tiled processing of graphs#183
Apply tiled functionality for tiled processing of graphs#183JoOkuma merged 17 commits intoroyerlab:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #183 +/- ##
==========================================
+ Coverage 88.02% 88.22% +0.20%
==========================================
Files 50 51 +1
Lines 3474 3593 +119
Branches 601 617 +16
==========================================
+ Hits 3058 3170 +112
- Misses 250 254 +4
- Partials 166 169 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I found it helpful to have a |
|
@JoOkuma Sure, I'll review this in a couple of days! |
yfukai
left a comment
yfukai
left a comment
There was a problem hiding this comment.
Almost all looks nice to me! I'm just afraid that the logic around eps might not work as expected if start and end can accept floats (since the tile size may be arbitrary small depending on the length unit). If those are ints as noted as type annotations, maybe we can simply use -1 or +1 to switch exclusive / inclusive slices.
| eps = 1e-8 # adding eps because np.arange is right exclusive | ||
| tiles_corner = list( | ||
| itertools.product( | ||
| *[np.arange(s, e + eps, t).tolist() for s, e, t in zip(start, end, tiling_scheme.tile, strict=True)] |
There was a problem hiding this comment.
Since start and end are ints, maybe we can do something like:
It may also be useful to note whether indices are inclusive or exclusive.
| *[np.arange(s, e + eps, t).tolist() for s, e, t in zip(start, end, tiling_scheme.tile, strict=True)] | |
| *[np.arange(s, e+1, t).tolist() for s, e, t in zip(start, end, tiling_scheme.tile, strict=True)] |
There was a problem hiding this comment.
Or if we accept the float as start and ends, epsilon should be relative to its values? (Since this may not work as expected if e-s<1e-8)
There was a problem hiding this comment.
@yfukai, np.nextafter ornp.spacing created other errors, I went for
np.arange(s, e, t).tolist() if s != e else [s]
What do you think? The eps was a hack for when s was equal to e and np.arange would fail.
| for corner in tiles_corner: | ||
| # corner considers the overlap, so right needs to be shifted by 2 * o | ||
| # -1e-8 is because tracksdata filter slicing is inclusive | ||
| slicing_without_overlap = tuple(slice(c, c + t - 1e-6) for c, t in zip(corner, tiling_scheme.tile, strict=True)) |
|
I'm sorry to be late, this is actually my first requested review |
|
Hi, @yfukai, thank you for the review, and don't need to apologize. I changed The tiling can be floats, so I changed the typing to reflect that. I also added a test to check if the scale is affecting the slicing, and I had to change the The two
Let me know if something is not clear |
|
It seems there's an unrelated issue that broke the CI, I'll open another PR to fix it. |
|
Hi @JoOkuma, thanks! I found out np.nextafter to swap the inclusive and exclusive ranges. Would it be safer to replace |
Hi @yfukai, thanks for the input. I didn't know this function. Using For the other use case, either that or I'm considering reverting this last commit 6988d91 and leaving it as it is. What do you think? |
|
If that looks fine okey, this PR itself looks good to go for me! |
This reverts commit 6988d91.
|
I still believe there's a edge case that only JoOkuma#4 can handle... I'll change the PR pointing to main. |
3 participants
@yfukai, could you review this PR?
I have a function that cannot handle the whole graph, so I'm adding functionality to chunk the data.
Because we might have tile artifacts, it needs an overlay. The only way I thought of being able to do this properly is by providing two graphs to the
MapFunc. I also considered providing the tile coordinates in case a user might want to slice the image at that position.