Normalized cell divisions feature #69
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stefanhahmann
force-pushed
the
normalized_cell_divisions_feature
branch
from
c1d7e11
to
ba9fda6
Compare
…tFeatureUtils and respective tests for it
stefanhahmann
force-pushed
the
normalized_cell_divisions_feature
branch
from
efae600
to
eec08ce
Compare
maarzt
reviewed
Feb 12, 2024
There was a problem hiding this comment.
Good code. Good unit tests. But I still see two problem:
- A "+ 1" is needed for the computation of the branch duration to make it more consistent.
- The algorithm for the computation of the branch duration and number of leaves is not efficient. You recompute similar values a lot. For example when computing the cell division frequency for this graph:
A
/ \
B C
/ \
D E
The duration for branch spot D and E will be computed 3 times. The duration of branch spot C is computed 2 times. The same is done for the number of leaves. It would make sense to cache intermediate results.
src/main/java/org/mastodon/mamut/feature/branch/BranchSpotFeatureUtils.java
Outdated
Show resolved
Hide resolved
…potFeatureUtilsTest
…mputer * Uses caching of values that have been already computed to avoid redundant computation * Tree is only iterated trough once * Similar approach as e.g. in BranchNLeavesFeature
Thanks for the advice.
|
maarzt
reviewed
Feb 20, 2024
.../org/mastodon/mamut/feature/branch/divisions/BranchCellDivisionFrequencyFeatureComputer.java
Outdated
Show resolved
Hide resolved
.../org/mastodon/mamut/feature/branch/divisions/BranchCellDivisionFrequencyFeatureComputer.java
Outdated
Show resolved
Hide resolved
…d of ObjPropertyMap * Reasoning: ObjPropertyMap supports undo/redo, which is not required for a value cache that is only used during feature computation. Thus presumably RefObjectMap is a bit more performant
# Conflicts: # README.md
…settableFeatureComputer to an AbstractCancelableFeatureComputer * Reasoning: partial re-computation of this feature is difficult, since results along the tree depend on each other.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds the new feature normalized cell division
Resolves #64