Closed 1minus1 closed 3 years ago
There is good discussion here on the topic of pushing porespy's 3D metrics upstream to scikit-image. https://github.com/scikit-image/scikit-image/issues/4679
Hi @1minus1. I wrote the region_props_3D function back before scimage's version worked on 3d images, and I also did it in a bit of a hurry for a project that I was working on, so it's not exactly solid. Ironically, the very next release of skimage had 3d capability, rendering my effort mostly wasted. I never really finished or polished my version because of this. For instance, I never figured out how to defer the actual calculations. I'm not sure how skimage does it. I have some experience now with Dask, which can defer things, but not sure if that is the best approach.
Anyway, to address your issue, what should we do in porespy? Should we consider removing the function entirely? What metrics does it calculate that skimage does not? I haven't read through the link you provided about moving some functionality 'upstream' to skimage, but I'm guessing that they don't want to add such esoteric info to their function as they aim for generalization.
At this point, I don't need any changes to resolve my issue since what I need is in scikit-image. It sounds like the best approach would be to push the relevant metrics from porespy into scikit-image, and/or use the forthcoming user-defined property capability to extend what's needed in porespy (and maybe that will give you caching for free?). It sounds like there is willingness on their side to enable this.
@jgostick Do you think it might be reasonable to rewrite the region_props_3D function to directly use more of the skimage API where applicable. I am thinking this would allow for flexibility to implement esoteric measures that porous material folks care about in the future within porespy and anything that can be generalized can be pushed upstream to skimage.
@1minus1 and I can contribute as necessary.
I haven't looked at this function in ages, but I will try to take a look soon. The first thing I'll do is figure out how to 'defer' the calculation of our props. Then we can address the duplications with the latest stuff in skimage.
At a meeting today, we realized that maybe it's time to move to PoreSpy V2.0. This would allow us to make some breaking changes, including removing many of the redundant values from this function.
This has actually been implemented in porespy now. It was much easier to do that I thought.
Currently, I think regionprops_3D calculates all structural parameters when it is called. I have a case where I only need bbox (which is offered by scikit-image.measure.regionprops). After digging, I solved this issue by reverting to scikit-image.measure.regionprops, and found that they cache all the properties and calculate them on demand rather than doing it all upfront, surely doing lots of unnecessary work. In my case, it wasn't just performance, but kept me from doing what I wanted, since I get an error in the calculation of convex hull and my code crashes.
The first two examples of extracting the bboxes for each region in the code below (using scikit-image.measure.regionprops and then by extracting the bbox method from that method) take the same amount of time, whereas regionprops_3D fails with the error shown below.
More discussion: https://github.com/scikit-image/scikit-image/issues/4679