Add ability to detect "families"#551
Add ability to detect "families"#551freemansw1 wants to merge 23 commits intotobac-project:RC_v1.7.0from
Conversation
# Conflicts: # tobac/tests/test_utils.py # tobac/utils/general.py # tobac/utils/internal.py
# Conflicts: # tobac/feature_detection.py
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## RC_v1.7.0 #551 +/- ##
=============================================
+ Coverage 64.84% 65.75% +0.91%
=============================================
Files 27 31 +4
Lines 3985 4135 +150
=============================================
+ Hits 2584 2719 +135
- Misses 1401 1416 +15
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Linting results by Pylint:Your code has been rated at 8.36/10 (previous run: 8.36/10, +0.00) |
|
Blocked by #553 . Also, need to add in a test and handling if there are no families. |
|
@freemansw1 I'll be happy to review this after #554 is merged |
|
Some overall thoughts before I do an in depth review: First off, really nice addition! On your particular points, the documentation is nice, but there is nothing in the user guide section (the same is true for merge/split actually), and the docstring for General thoughts:
I also noticed that the PBC labelling at current only uses connectivity=1 for connecting labels across borders, but I can fix that as part of the feature detection refactoring |
|
bug to investigate/test: grid output increments rather than carrying through 0 values in label with multiple times |
This has been resolved with the latest commit. |
| ) | ||
|
|
||
| else: | ||
| # TODO: integrate 3D stats - mostly around center coordinates - need the functions in tobac proper |
There was a problem hiding this comment.
Came across an issue when testing this with model data, do you want to implement this for the initial release?
| # TODO: deal with dim order for 3D | ||
| if is_3D: | ||
| if "vdim" not in rows_at_time: | ||
| raise NotImplementedError( |
There was a problem hiding this comment.
We could support this most easily by just reducing the mask using "any" along the vertical dimension
| ) | ||
|
|
||
| rows_at_time["vdim_adj"] = np.clip( | ||
| int(rows_at_time["vdim"] + 0.5).astype(int), a_min=0, a_max=v_max - 1 |
There was a problem hiding this comment.
Raises TypeError: cannot convert the series to <class 'int'>, fix:
| int(rows_at_time["vdim"] + 0.5).astype(int), a_min=0, a_max=v_max - 1 | |
| (rows_at_time["vdim"] + 0.5).astype(int), a_min=0, a_max=v_max - 1 |
| if target == "minimum": | ||
| mask = in_arr < threshold | ||
| elif target == "maximum": | ||
| mask = in_arr > threshold |
There was a problem hiding this comment.
This raises the issue of whether we consider the threshold inclusive again. I lean towards yes, but then we would have to change the threshold value in identify_feature_families_from_segmentation
| def identify_feature_families_from_data( | ||
| feature_df: pd.DataFrame, | ||
| in_data: xr.DataArray, | ||
| threshold: float, |
There was a problem hiding this comment.
Threshold should be optional if target=="bool"
| points_list = ( | ||
| rows_at_time["hdim_1_adj"].values, | ||
| rows_at_time["hdim_2_adj"].values, | ||
| rows_at_time["vdim_adj"].values, | ||
| ) |
There was a problem hiding this comment.
This leads to an error if the order of dimensions is different (e.g. z, y, x). Need to use the vdim axis to insert vdim in the correct position for indexing
|
Have been testing this out with some 3D model data, and hacking fixes to 3D bugs along the way, two main thoughts:
|
|
Found an error which occurs if you try to detect families with an input dataframe that already has a |
I've talked about this before; this is (finally) the code for family detection (family tracking still to come). This code links features together in space.
For the reviewers (I'm leaning toward two reviewers given that this is a new concept for tobac), please check the following:
Note: this is targeted at a new branch,
RC_v1.7.0.@w-k-jones and/or @JuliaKukulies are you up for reviewing? I know this is for v1.7.0, but I'd like to keep this moving so I can implement family tracking soon.