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

Add ROI name prefix to tiled region FOV names #66

Merged
merged 3 commits into from
Mar 29, 2022
Merged

Conversation

alex-l-kong
Copy link
Contributor

What is the purpose of this PR?

In preparation for the tiled region interactive overlay, we need to address the underlying names of the FOVs generated for each tiled region. Right now, they are all in the form R{m}C{n}, however this means that R1C1 will be indistinguishable from ROI1, ROI2, etc. Adding the ROI prefix will help disambiguate these (ex. if the ROI name is region1, then the FOVs generated will be named region1_R1C1, region1_R2C1, etc.).

Additionally, we need to standardize the use of roi as a variable identifier throughout the tiled regions functions. Right now, we use fov pretty generously. However, it will be easier to follow if we use roi.

How did you implement your changes

See above.

@alex-l-kong alex-l-kong self-assigned this Mar 28, 2022
@alex-l-kong
Copy link
Contributor Author

On second thought, I don't think it's worth forcing region to roi. However, any instance where fov should be region will still need to be fixed.

@alex-l-kong alex-l-kong requested a review from ngreenwald March 29, 2022 01:16
Copy link
Member

@ngreenwald ngreenwald left a comment

Choose a reason for hiding this comment

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

Nice, looks good

@ngreenwald ngreenwald merged commit 8bcf6ff into main Mar 29, 2022
@ngreenwald ngreenwald deleted the tiled_region_name branch March 29, 2022 05:58
@camisowers camisowers added the enhancement New feature or request label Sep 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants