-
Notifications
You must be signed in to change notification settings - Fork 0
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
Extract and replace data in overlapping regions between tiles #20
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #20 +/- ##
==========================================
+ Coverage 95.20% 95.54% +0.33%
==========================================
Files 6 6
Lines 626 673 +47
==========================================
+ Hits 596 643 +47
Misses 30 30 ☔ View full report in Codecov by Sentry. |
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.
Hi Igor, looks good to me. I'm just confused about how does get_local_overlap_coordinates
work. Once I understand this bit, I'll complete the review with the tests.
# Check for overlap in the x and y dimensions | ||
# and that the tiles do not have the same tile_id | ||
# and that the tiles are from the same channel | ||
if ( |
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.
Took me a while to parse these conditions 🤣
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.
They're a mouthful!
tile_j: Tile | ||
The second tile. | ||
""" | ||
self.coordinates: npt.NDArray = overlap_coordinates |
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.
Oh nice didn't know about numpy.typing.NDArray
... I was just using np.ndarray
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's very handy! You can also specify the specific type the array is holding.
self.coordinates: npt.NDArray = overlap_coordinates | ||
self.size: List[npt.NDArray] = [overlap_size] | ||
self.tiles: Tuple[Tile, Tile] = (tile_i, tile_j) | ||
self.local_coordinates: List[Tuple[npt.NDArray, npt.NDArray]] = [ |
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.
Is this datatype (List[Tuple[npt.NDArray, npt.NDArray]]
) convenient for you somewhere else? 😮
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 later understood that the tuple
level is for the resolution 🙂
Can we have a comment unpacking the hierarchies of local_coordinates
?
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.
Can do!
brainglobe_stitch/tile.py
Outdated
resolution_pyramid = self.tiles[0].resolution_pyramid | ||
|
||
for i in range(self.coordinates.shape[0]): | ||
if self.tiles[0].position[i] < self.tiles[1].position[i]: |
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.
Why are we referring to the first and second tile? 🤔
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 am understanding that the list of tiles is calculated once outside the creation of the Overlap
object, and so they will have fixed positions... how do the first and second tile relate to the local coords of any overlap?
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.
Each Overlap
is made up of two tiles. There are two separate but connected sets of coordinates that we keep track of, the "global" coordinates representing where the overlap lies in the final fused image, and the "local" coordinates which hold information on where the overlap lies in the Tile
it originates in. The get_local_overlap_coordinates
method checks which tile is "first" in each dimension to see if the overlap is at the beginning or the end for that specific dimension. This later allows us to extract, and modify the data stored in each overlap prior to fusing the entire image.
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.
Oh wow I would have never guessed that 😂
) | ||
self.size.append(self.size[0] // resolution_pyramid[j]) | ||
|
||
def extract_tile_overlaps( |
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 you think that these two methods will be useful for the plugin as well (and not uniquely to testing)?
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.
They'll be used in upcoming PRs for replacing overlapping areas after interpolating the stitched area.
@@ -1,4 +1,106 @@ | |||
from brainglobe_stitch.tile import Tile | |||
from typing import List, Tuple |
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.
Don't know if you only use tox
for testing, but with pytest
the tests do not run. I think the pyproject.toml
file needs a change:
[tool.pytest.ini_options]
addopts = ". --cov=brainglobe_stitch"
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 can't seem to replicate this. What's the error?
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.
No idea, now it works without any issue 🤷🏻♀️
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.
LeGiTiMate ✅
Before submitting a pull request (PR), please read the contributing guide.
Please fill out as much of this template as you can, but if you have any problems or questions, just leave a comment and we will help out :)
Description
What is this PR
Why is this PR needed?
In order to correct for brightness differences, and interpolate over the stitches in the final fused images it's important to keep track of which tiles overlap and the coordinates of the overlap in both
local
(relative to the tile itself) andglobal
(relative to the final fused image) coordinates.What does this PR do?
brainglobe_stitch/tile.py
: Introduced theOverlap
class to store and manage information about the overlap between two tiles, including methods to calculate local overlap coordinates and extract/replace overlap data.brainglobe_stitch/image_mosaic.py
: Updated theImageMosaic
class to include anoverlaps
attribute and methods to calculate overlaps between tiles. This involved modifying the constructor,load_mesospim_directory
, andstitch
methods, and adding thecalculate_overlaps
method.tests/test_unit/test_tile.py
: Added new tests for theOverlap
class, including tests for initialization, local coordinate calculation, overlap data extraction, and data replacement. These tests ensure that the overlap functionality works correctly.How has this PR been tested?
Tests added to cover the new functionality.
NOTE:
To test the functionality locally you must install Fiji and also add BigStitcher to the update sites. This can be done using the GUI see here or via the command line after installing Fiji.
./ImageJ-linux64 --update add-update-sites "BigStitcherUpdate" "https://sites.imagej.net/BigStitcher/"
./ImageJ-linux64 --update update
Replace ImageJ-linux64 with the path to the local ImageJ executable.
Three different datasets are available for testing:
Is this a breaking change?
No
Does this PR require an update to the documentation?
Yes, will come in subsequent PR.
Checklist: