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

Remove file extensions from fovs in generate_cell_data #311

Closed
wants to merge 8 commits into from

Conversation

alex-l-kong
Copy link
Contributor

@alex-l-kong alex-l-kong commented Oct 28, 2020

What is the purpose of this PR?

Addresses and closes #303. As it stands, the user cannot specify fovs with file extensions in generate_cell_data. This PR will fix that.

How did you implement your changes

We simply remove the file extensions as a precaution before running the rest of generate_cell_data. Because we're seeing this functionality being used in several places, we'll branch this off from extract_delimited_names into a separate function. By extension, this means we'll need to update the places in the repo where we call this.

@alex-l-kong alex-l-kong self-assigned this Oct 28, 2020
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.

See my comments about automatically removing delimiters when not set to None; I think it's going to cause problems.

If you didn't already, can you do a control for extract_delimited_names and see if there are any other locations where we use it?

ark/segmentation/marker_quantification.py Outdated Show resolved Hide resolved
ark/segmentation/marker_quantification.py Outdated Show resolved Hide resolved
ark/utils/io_utils.py Outdated Show resolved Hide resolved
@alex-l-kong
Copy link
Contributor Author

Verified that this passes wtih MIBITiff = True in Segment_Image_Data.ipynb.

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.

Looks good. I think it makes sense to hold off on integrating this until the jupyter notebook testing is up and running. Partly to give the high school students more time, and also because of all the different possible permutations of mibi-tiffs, multiple channels, etc, that aren't currently tested

print(f"These files still have \".\" in them after file extension removal: "
f"{','.join(bad_names)}, "
f"please double check that these are the correct names")
warnings.warn("remaining periods in file names")
Copy link
Member

Choose a reason for hiding this comment

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

Why have both a print statement and a warning? Does one not show up in certain locations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

warnings.warn doesn't actually print the desired warning message, only provides something that you can catch and do something with. I didn't know if we were planning to write code to explicitly catch these in the future, so I left them in.

You're right, though: we need to stick to one or the other. I should open an issue that addresses these inconsistencies. My opinion is that we should drop warnings.warn and simply do print statements. Normally, I would go the other route, but these functions that throw warnings are mostly used directly in the notebooks, and we don't want to handle the warnings in the notebooks themselves.

@alex-l-kong
Copy link
Contributor Author

The functionality in this branch has moved over to #318, closing.

@alex-l-kong alex-l-kong deleted the mibitiff_cell_mat branch November 11, 2020 20:11
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.

generate_cell_table fails if .tiff extensions are in supplied FOV names
2 participants