Skip to content
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

Adds isolation in 2d to segmentation file. #76

Draft
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

kelcyno
Copy link
Collaborator

@kelcyno kelcyno commented Feb 18, 2022

This is just an addition to the main tobac segmentation file. The additional parameter is ISO_dilate, a value by which the 2d feature is dilated in order to find the number of neighbors. 1 Neighbor value indicates the feature (which was dilated) does not have a neighbor. Values greater than 1 are the number of neighbors in addition to the base feature. Non-positive values indicate a feature area was not created for the feature id and thus no valid neighbors.

@freemansw1 freemansw1 self-requested a review February 18, 2022 21:30
@freemansw1
Copy link
Member

It looks like the CI failed due to switching between spacing and tabs for indents. I believe that the rest of the code uses four spaces to denote indentation rather than tabs. Looking at your code, I suspect that that may be the only issue?

Updated for spacing errors
@w-k-jones w-k-jones marked this pull request as ready for review March 4, 2022 15:18
@w-k-jones w-k-jones added this to the Version 1.5 milestone Mar 4, 2022
@@ -25,13 +25,15 @@ def segmentation_timestep(field_in,features_in,dxy,threshold=3e-3,target='maximu
method: string
flag determining the algorithm to use (currently watershedding implemented)
max_distance: float
maximum distance from a marker allowed to be classified as belonging to that cell
maximum distance from a marker allowed to be classified as belonging to that cell
ISO_dilate: int
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is 8 the default here? I'm not opposed to the value, I'm just curious.

@@ -120,6 +123,33 @@ def segmentation_timestep(field_in,features_in,dxy,threshold=3e-3,target='maximu
else:
raise ValueError('unknown method, must be watershed')

#create isolation, currently only available for 2d tracking
if field_in.ndim==2:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to make this entirely optional? Is running the isolation here necessary for everyone, or would it be better to have a parameter in place to turn it on/off?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the question here ultimately is: what's the performance penalty?

@freemansw1
Copy link
Member

I've added some comments to the code, but generally I'm happy here. I think we will want to wait to merge this until after #78 , but once that's in and this is updated to match the new style, I'm happy to merge this.

@w-k-jones w-k-jones self-requested a review March 16, 2022 20:31
@w-k-jones
Copy link
Member

Overall this looks good, but I have a couple of thoughts/requests:

  1. Like Sean, I think it would be better to have this as an optional parameter to calculate, rather than being run for everyone. Perhaps it would be good to set the default value of "ISO_dilate" to 0 and only calculate the isolation metric if a value greater than 0 is provided?
  2. Could this be run separately from segmentation? I.e. could this be run as a separate analysis function on a previously calculated segmentation mask. This would negate the need for the first point, and allow the effects of different ISO_dilate values to be compared more easily

The merge conflict is just due to formatting, so we can fix that easily before merging.

@w-k-jones w-k-jones self-assigned this Mar 25, 2022
@freemansw1 freemansw1 linked an issue May 15, 2022 that may be closed by this pull request
kelcyno added 2 commits June 2, 2022 15:28
Merge/split as postprocessing step.
@freemansw1 freemansw1 self-requested a review June 5, 2022 15:45
@freemansw1 freemansw1 removed this from the Version 1.5 milestone Sep 12, 2022
@freemansw1 freemansw1 marked this pull request as draft November 16, 2022 16:00
@freemansw1
Copy link
Member

I've marked this PR as a draft for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Isolation metric in tobac
3 participants