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

using io.imsave(png_fname, img_as_ubyte(some_binary_mask)) to save compressed mask #294

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

nanli-emory
Copy link
Collaborator

@nanli-emory nanli-emory commented May 6, 2024

Try to fix #291:

  1. created a saveCompressedMask method in ImageBase as an until function.
  2. change all the io.imsave to saveCompressedMask if mask is grayscale.

@CielAl
Copy link
Contributor

CielAl commented May 9, 2024

Just something semi-relevant: I actually have a proposal that, perhaps we can keep all module function's naming convention to be the camel case, but everything else follow python's snake case:

  • we can't really change the module function name, because essentially that changes the configs and forces users to make massive modifications on their end.
  • Currently this project mixes different naming conventions from different developers and one day it will makes the codes look like a potty lol.

Also, just my personal opinion: I am kinda against piling up methods to BaseImage.py and the BaseImage class itself. In my opinion what semantically matches the purpose of BaseImage is to read anything from the file, and receiving anything from the module (e.g., keeping all masks and stats in the dict). A conditionally saving of an image file itself can really go to a standalone util module, along with all other possible miscellaneous functions in future.

@jacksonjacobs1
Copy link
Collaborator

Thanks Nan.

@CielAl I'd like to avoid creating a new module to store a single method or future "miscellaneous" methods. Ideally future methods should be sorted into existing or (if needed) new modules by functionality.

I think saveCompressedMask() would fit nicely in the SaveModule?

@CielAl
Copy link
Contributor

CielAl commented May 14, 2024

Thanks Nan.

@CielAl I'd like to avoid creating a new module to store a single method or future "miscellaneous" methods. Ideally future methods should be sorted into existing or (if needed) new modules by functionality.

I think saveCompressedMask() would fit nicely in the SaveModule?

Yes, you are right. This should indeed work better since this saving behavior has one dedicated purpose and we don't have much needs to refactor all modules at the moment.

Just some concerns (but neglectable), to save a numpy array with bits=1 would require it to be binarized (except mode "1" in PIL). Could contribute to some unhandled error with not-straightforward error message. We could perhaps wrap the entire image/mask saving into one function with compression as a flag, and compression flag is only applicable if the image is binarized (e.g., ignore if not). In this case, we wouldn't see different function calls when you save a thumbnail, a grayscale image (e.g. color deconv), and a binary mask. Easier to manage in future.

@CielAl
Copy link
Contributor

CielAl commented May 14, 2024

Another potential risk is that, because many modules need to save the mask explicitly, if we fit it into SaveModule then it creates a dependency between modules which might need to be independent to each other.
But it should be managable if it is documented.

(anyhow, not major concerns so whatever easier is good :) )

@nanli-emory
Copy link
Collaborator Author

Hi @jacksonjacobs1 and @CielAl,

Please review and confirm the changes.

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.

Apply run length encoding while saving masks.
3 participants