smistad / FAST

A framework for high-performance medical image processing, neural network inference and visualization
https://fast.eriksmistad.no
BSD 2-Clause "Simplified" License
433 stars 101 forks source link

Bug fix regarding import/export not being executed #142

Closed andreped closed 3 years ago

andreped commented 3 years ago
  1. Added fix to ensure that import/export of low and high-res images are being properly exported in C++.
  2. Added small fix for RGB/BGR switch in ImageExporter. 3. Added option to set attributes through text pipelines for a selection of classes. 4. Added method to set the tissue acceptance threshold (which was hard-coded to 0.5). 5. Lowered the image pyramidal plane size threshold to accept lower image planes in the stitcher, which were relevant in a study. I left a comment in the code if we want to switch back and compare in the future. 6. Also did some minor refactoring to keep the code more consistent on a selection of files.

Tested this using FastPathology on Ubuntu, both through C++ and runPipeline. Both worked fine.

(1) The bug was found comparing the specific classes against HDF5TensorExporter/HDF5TensorImporter, which seemed to work great. Using these two scripts as reference I noticed some minor differences.

I found that adding setModified(true) was quite important. I'm not really sure why not just using mIsModified=true did not work. Perhaps there was something I forgot to add to the header. Now there are occasions where I am using both setModified(true) AND mIsModified=true, where the latter is then probably redundant? Perhaps it is easier for you to say what is correct, so I just left it like this for you to evaluate.

I am also assuming that there should be an override for the execute method if the parent class has an execute method already. I noticed that this was done for the HDF5TensorExporter, and not the specific scripts I was working with. However, in this case it did not really seem to matter.

(2) I noticed that when creating thumbnail images from WSIs, that the exported images had swapped RGB channels. I proposed a simple fix that I have used earlier, but perhaps there is a more elegant solution. This should reproduce the bug: auto importer = WholeSlideImageImporter::New(); importer->setFilename(path_to_wsi); auto currImage = importer->updateAndGetOutputData();

auto access = currImage->getAccess(ACCESS_READ); auto input = access->getLevelAsImage(currImage->getNrOfLevels() - 1)

auto exporter = ImageExporter::New(); exporter->setFilename(path_to_output.png); exporter->setInputData(input); exporter->update();

(5) Perhaps this threshold should even be a hyperparameter one could set? But for now, I just manually lowered them. Did not notice any real difference using runPipeline and FastPathology on Ubuntu, for A05.svs and a larger 40x WSI from a local cohort. But perhaps overall runtime of high-res models will be slower? Right? But perhaps this is okay for now?

Let me know if there are any requests.

smistad commented 3 years ago

Adding mIsModified=true has no effect, because it "shadows" the original variable from the ProcessObject class. The correct use is setModified(true); in functions that modify the PO.

It is true that openslide reads data as BGR, however that should be handled in the getLevelAsImage function: https://github.com/smistad/FAST/blob/master/source/FAST/Data/Access/ImagePyramidAccess.cpp#L182

andreped commented 3 years ago

I agree. I have adjusted for your comments in the most recent commit.

andreped commented 3 years ago

I have made some futher modifications. I have marked the new changes in bold in the initial comment.

andreped commented 3 years ago

I have adjusted for the comments above.