search

PMEAL / porespy

A set of tools for characterizing and analying 3D images of porous materials
https://porespy.org
MIT License
298 stars 99 forks source link

`tortuosity_fd` might be doing too much #746

Open ma-sadeghi opened 1 year ago

ma-sadeghi commented 1 year ago

Here's the list of things that tortuosity_fd currently does:

I think it should only do the last. Let me explain: tortuosity_fd is almost 100 lines of code, which itself is sort of a red flag that it needs to be refactored.

  1. Checking whether the image is percolating is just too much babysitting, I propose either adding a helper function is_percolating, which would then be called inside tortuosity_fd and optionally throw an Exception. Also, checking whether the image is percolating is one thing, but fixing the image for the user, I think is too much. For the very least, I think we should stop fixing the image inside tortuosity_fd, but rather inform the user that they need to do something.

  2. Running Fickian diffusion on an image with prescribed Dirichlet boundary conditions could be its own standalone function, accepting a binary image and two boundary conditions, and optionally an openpnm solver object for more fine-grained control. This function outputs only a concentration map.

  3. Calculating tortuosity from the concentration map should be its own function. In fact, we have already written this function and been using it in the machine learning project, so we might as well reuse it elsewhere.

  4. Finally, we will write an example notebook that explains how to weave all this together to form a coherent workflow for calculating tortuosity of an image.

@jgostick What do you think?

PS. The main reason I think we should do this, apart from tortuosity_fd being very long and doing too much, is that simulations is the most under-developed module in porespy. If we're going to expand it, it needs to be more porespy-like, i.e., simple functions that together form a pipeline for doing a particular task, and currently it isn't.

jgostick commented 1 year ago

This all seems reasonable. As for the blind pores and percolating checks, we for sure still need to do these checks...it's not clear that we should attempt to fix them though. On the one hand this is a bit too much interference from porespy, but on the other hand, it's very easy to fix and requires no interpretation...just fill the blind/nonpercolating regions.

In terms of refactoring, a function that computes the flux given a gradient is a good idea, and maybe we should add something like prep_for_transport (name tbd) which does the fixes (fills blind and non-percolating pores). Both of these functions could be called from within the tortuosity_fd function, thereby reducing the code a lot.

Another point, we could set the solid phase to g_D = 0.0001, while the void space is has g_D = 1.00. Then all these checks are no longer needed. This would increase the run time though since we're solving for transport on the solid voxels.