Add progressbar to prism forward gravity calculations

[mdtanker]

Description

This PR adds an optional progress bar to the forward gravity calculation of prisms_gravity. It uses numba_progress, and is called with a flag progressbar=True, which defaults to False. Currently it only works properly with parallel=True.

Relevant issues/PRs

Fixes #312

[welcome]

💖 Thank you for opening your first pull request in this repository! 💖

A few things to keep in mind:

• Authorship Guidelines: Our rules for giving you credit for your contributions, including authorship on publications and Zenodo archives.
• Code of Conduct: How we expect people to interact in our projects.

⭐ No matter what, we are really grateful that you put in the effort to do this! ⭐

[santisoler]

Looking awesome @mdtanker !

I left some small suggestions to improve the code. Nothing critical, just a few ideas.

Besides those, the only thing we need is to write some tests for this. Feel free to ask for help if you need some!

[mdtanker]

Thanks for all the suggestions @santisoler. I've added them all in and have pushed the new commits. Any guidance for the tests would be appreciated!

[santisoler]

Thanks for applying those changes @mdtanker ! This is looking great.

Regarding the tests. Harmonica test functions live in harmonica/tests. Each test function should be named test_... so pytest can know that it should run it.

There are different types of tests that we can run. The most important ones in Harmonica are the ones that check if a computation is producing accurate results. These are critical for ensuring that our tools compute what they meant. In harmonica/tests/test_prism.py we have several test functions for the prism_gravity function. I would add a function in there that checks if the results when passing progressbar=True are the same as the ones obtained when passing progressbar=False.

The same type of function should be added to harmonica/tests/prism_layer.py but for the prism layer.

You'll see that the test functions usually have a bunch of decorators (those lines that start with @). They are intended to use some of the nice tools provided by pytest like parametrization (running the same test under different scenarios) or fixtures (create sample objects that test functions use as input, like sample computation points or a sample collection of prisms). You can learn more about them in pytest docs: https://docs.pytest.org/en/6.2.x/parametrize.html

Besides, you'll find that we often use two different decorators:

• @pytest.mark.use_numba
• @run_only_with_numba

The first decorator should be added to every test function that tests a code written in Numba, while the second one is intended to be used on those test functions that take too long to run if Numba JIT is disabled.
Don't worry too much about the second one for now, but on every test function that will be computing the gravity of prisms use the first one.

Besides those there are other type of tests we should run, which are more intended to guarantee that the user interface works as expected. For instance, we need to check that the prism_gravity function raises a ImportError if numba_progress is not installed. Check out how we do it for pyvista:

harmonica/harmonica/tests/test_visualizations.py

Lines 121 to 128 in 6a30797

	@patch("harmonica.visualization.prism.pyvista", None)
	def test_prisms_pyvista_missing_error(prisms, density):
	"""
	Check if prism_to_pyvista raises error if pyvista is missing
	"""
	# Check if error is raised
	with pytest.raises(ImportError):
	prism_to_pyvista(prisms, properties={"density": density})

When you run make test you'll get a detailed report of which tests passes, which fails and also which lines are being tested and which aren't. The percentage of code tested is called test coverage. We aim to hit 100% test coverage, although sometimes it isn't possible. But always remember that 100% test coverage doesn't mean good tests. So we prioritize test quality over test coverage.

Feel free to ask for help. We can also add code to this branch if you need, so don't feel obliged to write every possible test.

[santisoler]

Yesterday I was running some benchmarks on this branch against the code we have in main to test if there was any performance hit on adding the progress bar.

Here I leave the script I used for benchmarking:

Details

import time
import numpy as np
import verde as vd
import harmonica as hm

region = (0, 100e3, -40e3, 40e3)
spacing = 1000

easting, northing = vd.grid_coordinates(region=region, spacing=spacing)

wavelength = 24 * spacing
surface = np.abs(np.sin(easting * 2 * np.pi / wavelength))

density = np.full_like(surface, 2700)

prisms = hm.prism_layer(
    coordinates=(easting, northing),
    surface=surface,
    reference=0,
    properties={"density": density},
)

region_pad = vd.pad_region(region, 10e3)
coordinates = vd.grid_coordinates(region_pad, spacing=spacing, extra_coords=1e3)

# Run forward modelling once so Numba can compile
print("\nRunning first time for compilation")
prisms.prism_layer.gravity(coordinates, field="g_z")

# Then benchmark it
print("\nStarting benchmark")

times = []
for i in range(5):
    print(f"Running iteration {i}")
    start = time.time()
    gravity = prisms.prism_layer.gravity(coordinates, field="g_z")
    end = time.time()
    times.append(end - start)

print(f"Results: {np.mean(times)} +/- {np.std(times)}")

I run it as it is with the code in main to have a reference, and with the code in this branch choosing progressbar=True and progressbar=False. The results I got were:

Type of benchmark	Time elapsed (s)
main branch	8.3 ± 0.4
progressbar=True	8.2 ± 0.8
progressbar=False	8.4 ± 0.4

These specific values depend on the hardware setting and your cpu load at the time of the benchmark

In conclusion, I don't see any appreciable performance hit, not even comparing this branch with main, and not even choosing progressbar=True or progressbar=False.

[mdtanker]

@santisoler thanks for the help with the tests. I've added test which I believe accomplish what we want. make test results are below.

----------- coverage: platform linux, python 3.9.0-final-0 -----------
Name                                                                              Stmts   Miss Branch BrPart  Cover   Missing
-----------------------------------------------------------------------------------------------------------------------------
/Users/home/tankerma/harmonica/harmonica/_version.py                                  3      0      0      0   100%
/Users/home/tankerma/harmonica/harmonica/constants.py                                 2      0      0      0   100%
/Users/home/tankerma/harmonica/harmonica/datasets/sample_data.py                     52      0      0      0   100%
/Users/home/tankerma/harmonica/harmonica/equivalent_sources/cartesian.py             88      0     24      0   100%
/Users/home/tankerma/harmonica/harmonica/equivalent_sources/gradient_boosted.py      79      0     30      0   100%
/Users/home/tankerma/harmonica/harmonica/equivalent_sources/spherical.py             66      0     12      0   100%
/Users/home/tankerma/harmonica/harmonica/equivalent_sources/utils.py                 29      0     14      0   100%
/Users/home/tankerma/harmonica/harmonica/forward/_tesseroid_utils.py                162      0     66      0   100%
/Users/home/tankerma/harmonica/harmonica/forward/_tesseroid_variable_density.py      67      0     16      0   100%
/Users/home/tankerma/harmonica/harmonica/forward/point.py                           103      0     22      0   100%
/Users/home/tankerma/harmonica/harmonica/forward/prism.py                            98      2     50      0    99%   141-142
/Users/home/tankerma/harmonica/harmonica/forward/prism_layer.py                     108      0     26      0   100%
/Users/home/tankerma/harmonica/harmonica/forward/tesseroid.py                        68      0     26      0   100%
/Users/home/tankerma/harmonica/harmonica/forward/utils.py                            57      0      8      0   100%
/Users/home/tankerma/harmonica/harmonica/gravity_corrections.py                      11      0      0      0   100%
/Users/home/tankerma/harmonica/harmonica/io.py                                       70      0     42      0   100%
/Users/home/tankerma/harmonica/harmonica/isostasy.py                                 18      0      2      0   100%
/Users/home/tankerma/harmonica/harmonica/synthetic/surveys.py                        34      0      6      0   100%
/Users/home/tankerma/harmonica/harmonica/visualization/prism.py                      31      2     12      0    95%   14-15
-----------------------------------------------------------------------------------------------------------------------------
TOTAL                                                                              1146      4    356      0    99%

================================================================================== short test summary info ===================================================================================
FAILED ../harmonica/forward/tesseroid.py::harmonica.forward.tesseroid.tesseroid_gravity
================================================================== 1 failed, 281 passed, 121 skipped, 56 warnings in 26.22s ==================================================================
make: *** [Makefile:26: test_coverage] Error 1

It resulted in 1 failure, which was on harmonica.forward.tesseroid.tesseroid_gravity so I don't think that is related. The tests had 99% coverage for prism.py. It was missing lines 141-142, the except statement in the following:

# Attempt to import numba_progress
try:
    from numba_progress import ProgressBar
except ImportError:
    ProgressBar = None

Here was my test for whether numba_progress was installed, not sure if I did this properly.

@patch("numba_progress.ProgressBar", None)
def test_numba_progress_missing_error():
    """
    Check if error is raised when progresbar=True and numba_progress package 
    is not installed.
    """
    prisms = [
        [-100, 0, -100, 0, -10, 0],
        [0, 100, -100, 0, -10, 0],
        [-100, 0, 0, 100, -10, 0],
        [0, 100, 0, 100, -10, 0],
    ]
    densities = [2000, 3000, 4000, 5000]
    coordinates = [0, 0, 0]
    # Check if error is raised
    with pytest.raises(ImportError):
        prism_gravity(
            coordinates, prisms, densities, field="potential", progressbar=True
        )

[santisoler]

Looking great @mdtanker!

The tesseroid test error is probably unrelated. If that's the case we will handle it on a separate PR, so don't worry.

It's ok not to achieve 100% of coverage in this case. The try: import... lines are hard to test: we would need to mimic a missing numba_progress at import time. It's a tricky problem and we don't get much out of it besides a not very meaningful 100%. So it's ok if we leave those lines untested. The test for the custom ImportError that prism_gravity raises is great though! I only have a small suggestion on it, check comment below.

As you can see now we are running CIs on this branch to check that everything passes. We would need to add numba_progress as a requirement for running the tests. Please edit the env/requirements-test.txt file and add it there. CIs are also complaining about code style, that one is easy: just run make format to autoformat the code to comply with PEP8.

Thanks again @mdtanker ! Let me know if you need any help!

[santisoler]

@mdtanker This is looking great. I think this is ready to be merged. Is there something else you want to add? We can always change things later, no problem.

[mdtanker]

@mdtanker This is looking great. I think this is ready to be merged. Is there something else you want to add? We can always change things later, no problem.

Merge away! Thanks for all the help.

[welcome]

🎉 Congrats on merging your first pull request and welcome to the team! 🎉

If you would like to be added as a author on the Zenodo archive of the next release, add your full name, affiliation, and ORCID (optional) to the AUTHORS.md file of this repository. Feel free to do this in a new pull request if needed.

We hope that this was a good experience for you. Let us know if there is any way that the contributing process could be improved.