-
Notifications
You must be signed in to change notification settings - Fork 1
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
Include phase cross correlation flow #10
base: main
Are you sure you want to change the base?
Conversation
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.
Main change needed is to make a new edge attribute to hold the new value, something like FLOW_DISTANCE, instead of overwriting the distance attribute. A couple other nitpicks.
images (np.ndarray): Raw images (t, c, [z], y, x). | ||
|
||
""" | ||
for node in candidate_graph.nodes(data=True): |
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.
unpack into node_id, data would be nicer to read
frame = node[1][NodeAttr.TIME.value] | ||
if frame + 1 >= len(images): | ||
continue | ||
loc = node[1][NodeAttr.POS.value] |
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 get rid of loc and use the size of the bbox to infer the number of dimensions
bbox[2] : bbox[5] + 1, | ||
] | ||
shift, _, _ = phase_cross_correlation(reference_image, shifted_image) | ||
node[1][NodeAttr.FLOW.value] = shift |
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.
What does this represent? Is it a single number? A vector? A vector at each pixel in the input image?
node[1][NodeAttr.FLOW.value] = shift | ||
|
||
|
||
def correct_edge_distance(candidate_graph: nx.DiGraph): |
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.
First, I strongly don't think this should overwrite distance, it needs its own attribute. Second, Jan and I removed the distance attribute (and updated Motile.EdgeDistance cost) 😆 so now this feature definitely needs its own attribute!
distance attribute is updated, by taking flow into account). | ||
|
||
""" | ||
for edge in candidate_graph.edges(data=True): |
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.
again, I prefer unpacking the tuple here rather than using [2] later
This reverts commit e480586.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10 +/- ##
==========================================
- Coverage 83.83% 77.13% -6.71%
==========================================
Files 12 13 +1
Lines 297 328 +31
==========================================
+ Hits 249 253 +4
- Misses 48 75 +27 ☔ View full report in Codecov by Sentry. |
Proposed Change
This PR primarily includes calculation of flow by phase cross correlation and corrects the edge distance feature by incorporating the now available flow. It is designed to be used as follows:
By calculating the flow separately and after constructing the candidate graph, allows the flexibility to replace this step by a flow coming from an alternate source (for example, a DL model).
Checklist
Go through these things before merge. Actions should run automatically to test them, but for information on how to run locally, see CONTRIBUTING.md.
Further Comments
Other minor changes include:
3.10
onwards (f75e60d) since the typing does not work for versions below.motile
from the github (6c27a49).BBOX
andFLOW
by default (210371f).