Adding a Median Filter for Non-Finite Values

[WilliamJudge94]

The following code has been added to Tomopy to allow for easy removal of non-finite values inside of a 3D array. The code iterates through each projection, finds non-finite values (nan/inf), and replaces them with the median value of the surrounding pixel values based upon the kernel size set by the user. The median value does not include non-finite value in its calculation.

Closes #564

[pep8speaks]

Hello @WilliamJudge94! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

• In the file source/tomopy/misc/corr.py:

Line 331:51: E127 continuation line over-indented for visual indent

Comment last updated at 2021-11-11 23:12:09 UTC

[WilliamJudge94]

I took a look at some of the failing tests and they are all due to "no module named tqdm" import errors. I would prefer to have a loading bar for these iterations. I have found a few examples of reconstructions with contain 4mil non-finite values. Taking upwards of 1-2 min to replace all values. If users are not given a loading bar they may think the program has stalled on them.

[prjemian]

Add tqdm to the environments (or other project requirements). It's a good addition that would provide such a progress bar.

[prjemian]

Also, on the developer side install and use flake8 before each commit/push to ensure your contributions are consistent with PEP8.

[carterbox]

Since tomopy is a library (not an application), it is out-of-scope to include and UI elements. If you want to provide feedback to the user, you may use the logging module from the standard library. Using the logging module allows end users to customize the verbosity and format of logging messages without editing tomopy's source code.

[WilliamJudge94]

That makes sense. Although would you allow me to 1) add in a "disable" variable for the tqdm UI 2) Along with the tomopy standard logging feature?

[carterbox]

Thanks for opening your first PR!

• As mentioned above, it is inappropriate to use tqdm in a library.
• Please add a test of median_filter_nonfinite() in test/test_tomopy/test_misc/test_corr.py. The test should be a simple example which checks that your function does what it claims to do.
• Add your function to __all__ at the top of the file
• Add your name to the authors at the top of the file

[carterbox]

would you allow me to add in a "disable" variable for the tqdm UI?

No.

The Why

If we included tqdm in our implementation, then tqdm would become a required dependency or lazy import when the end user could easily wrap this function in tqdm with the following:

for i in tqdm(range(len(data))):
    data[(i,)] = filter_median_nonfinite(data[(i,)])

while maintaining full control over the many formatting options that tqdm provides.

Alternative

filter_median_nonfinite can accept a callback function which is called once per iteration. This is very similar to what you have implemented in 040be55, but instead of passing a tqdm instance, you would pass a lambda function.

with tqdm(total=len(data)) as pbar:

    def callback():
        pbar.update(1)

    data = filter_median_nonfinite(data, callback=callback)

This keeps the tomopy codebase simpler and allows end-users to choose another framework besides tqdm.

[WilliamJudge94]

1. Since I have two main loops inside this median_filter function would you like me to have two different callbacks? One for each loop? Or would the solution detailed in 2) be sufficient?

2. Since I would need a way to pass an iterator_length and iterator_name
   into their callback would it be ok if I changed your callback function into a callback class? Something similar to this:

class CallbackClass():
    
    def callback(self):
        # Allow user to define this but have access to the iterator length and iterator_name
        
    def iterator_len(self):
        # pass in length
        
    def iterator_desc(self):
        # pass in name of loop

This allows me to define callback_loop1 = CallbackClass(), callback_loop2 = CallbackClass() for each loop defined by the function.

[WilliamJudge94]

Assuming these commits pass testing, the following is an example of how to properly run the tqdm progress bar with the median_filter_nonfinite() function. I can add this function implementation to the tomopy docs assuming you approve the current implementation.

from tomopy.misc.corr import median_filter_nonfinite
from time import sleep
import numpy as np

data = np.ones(shape=(1000, 1000, 1000))

for i in range(5000):
    x = np.random.randint(0, 100)
    y = np.random.randint(0, 100)
    z = np.random.randint(0, 100)
    data[z, x, y] = np.inf

with tqdm() as pbar:

    def callback(self):
        pbar.total = self.iterator_len
        pbar.set_description(self.iterator_desc)
        pbar.update(1)

        # Callback function is too fast leading to display errors in jupyter
        sleep(0.003)

    data = median_filter_nonfinite(data, callback=callback)

[carterbox]

Since I have two main loops inside this median_filter function would you like me to have two different callbacks? One for each loop? Or would the solution detailed in 2) be sufficient?

Why do we need two progress bars? Process one projection at a time so there is only one progress bar needed?

for p in projections:
    nonfinite_idx_store = _find_nonfinite_values_prj(p)
    for indices in zip(nonfinite_idx_store.transpose()):
        # apply filter
    pbar.update(1)

[carterbox]

Since I would need a way to pass an iterator_length and iterator_name
into their callback would it be ok if I changed your callback function into a callback class? Something similar to this:

You could define the callback as:

def callback(total, description):
        pbar.total = total
        pbar.set_description(description)
        pbar.update(1)

Then you don't need a custom class?

[WilliamJudge94]

I have allowed the median_filter_nonfinite() function to run on a single progress bar instead of two. In addition, I have allowed users to define a callback function that does not depend on a CallbackClass. Below is the proper usage of the callback variable.

with tqdm() as pbar:

    def callback(total, description, unit):
        pbar.total = total
        pbar.set_description(description)
        pbar.unit = unit
        pbar.update(1)

    data = median_filter_nonfinite(data, callback=callback)

[carterbox]

Thanks for your work @WilliamJudge94!