Skip to content

392 label and select distinct rois#406

Merged
JulietteFrancovich merged 51 commits intodevelopfrom
392_label_and_select_distinct_ROIs
Aug 13, 2025
Merged

392 label and select distinct rois#406
JulietteFrancovich merged 51 commits intodevelopfrom
392_label_and_select_distinct_ROIs

Conversation

@JulietteFrancovich
Copy link
Copy Markdown
Contributor

Created a class to label and select distinct ROIs based on a minimum pixel size and connectivity.
The class now returns a PixelMask that uses .combine from PixelMaskCollection. An alternative could be to return the PixelMaskCollection and use .combine afterwards. What do you think?

@psomhorst psomhorst changed the base branch from main to develop August 11, 2025 09:57
@psomhorst
Copy link
Copy Markdown
Contributor

@JulietteFrancovich I changed the base to merge to to develop.

Copy link
Copy Markdown
Contributor

@psomhorst psomhorst left a comment

Choose a reason for hiding this comment

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

I think the general design of this class is good. I think the structure is good. It has a separate function to do validation/conversion work in init. It splits setup and application.

The documentation is a bit minimal. Especially around the structure it could use more information. Also, it's not really explained what the selector does: selecting regions based on size.

The class should be a dataclass with some extra requirements.

The tests cover most use cases. I have some small suggestions to improve especially the readability.

Copy link
Copy Markdown
Contributor

@psomhorst psomhorst left a comment

Choose a reason for hiding this comment

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

Thanks for the improvements!

I have some more suggestions. Some of those are about how the docstrings render in the documentation. I pushed a single commit adding the class to the documentation. I added a footnote about rendering the docs locally to one of the comments.

The module name feels a bit clunky. from eitprocessing.roi.roi_size_filter import FilterROIBySize has too many ROIs in it.
Maybe eitprocessing.roi.size_filter or eitprocessing.roi.filter_by_size?

@JulietteFrancovich JulietteFrancovich force-pushed the 392_label_and_select_distinct_ROIs branch from 8c2503e to 9736a46 Compare August 11, 2025 14:12
@JulietteFrancovich JulietteFrancovich linked an issue Aug 12, 2025 that may be closed by this pull request
Copy link
Copy Markdown
Contributor

@psomhorst psomhorst left a comment

Choose a reason for hiding this comment

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

This looks great! I have a few more comments, but you can go ahead and merge if you think it's done.

I added a commit allowing PixelMaskCollection() without arguments. I think that is much cleaner than PixelMaskCollection({}).

I also added a commit to prevent a warning when comparing all-nan arrays with values.

PS: GitHub rebased the branch after develop was updated. You then merged that branch into your own. That results in many duplicated commits in the history. It is better to run "Pull (Rebase)" in VSCode or rebase locally and then force-push instead of rebasing on GitHub.

@JulietteFrancovich JulietteFrancovich force-pushed the 392_label_and_select_distinct_ROIs branch from 58129a9 to eab1993 Compare August 13, 2025 07:30
@JulietteFrancovich JulietteFrancovich merged commit ef7fbc1 into develop Aug 13, 2025
3 checks passed
@JulietteFrancovich JulietteFrancovich deleted the 392_label_and_select_distinct_ROIs branch August 13, 2025 07:31
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.

Create class to label and select distinct ROI regions

2 participants