choosehappy / HistoQC

HistoQC is an open-source quality control tool for digital pathology slides
BSD 3-Clause Clear License
263 stars 105 forks source link

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

Open nanli-emory opened 4 months ago

nanli-emory commented 4 months ago

Try to fix https://github.com/choosehappy/HistoQC/issues/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 commented 4 months ago

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:

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

Hi @jacksonjacobs1 and @CielAl,

Please review and confirm the changes.