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

Added the ability to set a custom stub cell value instead of np.nan #74

Merged
merged 8 commits into from
Mar 28, 2022

Conversation

freemansw1
Copy link
Member

This should resolve (most of) #62. Rather than set the stub/non-tracked cells as np.nan, tobac.tracking.linking_trackpy now accepts stub_set_number as a parameter for the value of cell to set the stub/non-tracked cells to, with a default value of -1.

@freemansw1 freemansw1 added the enhancement Addition of new features, or improved functionality of existing features label Feb 18, 2022
@w-k-jones w-k-jones added this to the Version 1.5 milestone Mar 4, 2022
@deeplycloudy
Copy link
Contributor

I tested this (after porting to v2.0-dev), and it resolves the float-coercion problem in #62.

By definition, the lists of segmentation IDs and feature IDs are separate, so I agree segmentation is a separate discussion.

Is "stub" a term in the public API, or a term internal to the implementation? For symmetry with other kwargs, I think I'd prefer something like cell_number_unassigned instead of stub_set_number. That mirrors cell_number_start.

@deeplycloudy
Copy link
Contributor

deeplycloudy commented Mar 4, 2022

Also, thinking about metadata: it would be good to capture the user's chosen stub/unassigned number as an attribute. I could propose how to do that for xarray, but not sure if iris supports such metadata.

I'm very interested in having the metadata. When working with a combined dataset (features, the feature mask, and cells) one would need to know which ID to ignore, and this function seems like the right place to add that metadata so it can be used appropriately downstream. While the user knows the ID when creating the dataset, that information is lost when those data are saved to an archive apart from the software.

@freemansw1
Copy link
Member Author

"stub" is indeed used in the public API (see tobac.tracking.linking_trackpy), although it definitely shouldn't be as the language isn't intuitive. I'll switch to cell_number_unassigned as you suggest here.

The issue with metadata here is actually with pandas rather than iris, as in v1.x, the feature/tracking data comes out as a pandas DataFrame. Pandas has the DataFrame.attrs dict, but it is described as experimental. For now, I'm going to add the selected unassigned cell value there as cell_number_unassigned.

I (or someone else) will need to port this to v2.x, but I'm hesitant to make any ports until we get to v1.5.

@freemansw1 freemansw1 requested a review from w-k-jones March 7, 2022 17:40
@freemansw1 freemansw1 modified the milestones: Version 1.5, Version 1.3 Mar 7, 2022
@freemansw1
Copy link
Member Author

I've moved this to the v1.3 milestone to match the issue (#62).

@freemansw1
Copy link
Member Author

Per our discussion last week, waiting on #78 to be merged before merging this.

@freemansw1
Copy link
Member Author

I've updated the formatting per #78

Copy link
Member

@w-k-jones w-k-jones left a comment

Choose a reason for hiding this comment

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

Happy for this to be merged

@freemansw1
Copy link
Member Author

Now that #89 has been merged, I need to update this PR to avoid the conflicts.

@freemansw1
Copy link
Member Author

I've updated it to avoid the conflict and I will re-request a review to keep with good practices before merging.

@freemansw1 freemansw1 requested a review from w-k-jones March 28, 2022 03:50
@freemansw1 freemansw1 merged commit 4f204e2 into tobac-project:dev Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Addition of new features, or improved functionality of existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants