Repository: legend-exp/pygama (language: python)

Issue:
Unit testing
Here's a to-do list, I'll kep it up-to-date. Good reference: https://scikit-hep.org/developer. Most of this will be implemented on the `refactor` branch only.

- [ ] Add micro-tests for each module/function separately. Read the [contribution guide (`refactor` branch)](https://legend-exp.github.io/pygama/refactor/contributing.html). Add @gipert as a reviewer for your pull requests. Assignments (put your name!):
  - `dsp` @iguinn 
  - `flow`
  - `hit` @wisecg 
  - `lgdo` @jasondet 
  - `math`
  - `pargen` @ggmarshall 
  - `raw` @jasondet 
  - `vis` @iguinn 
- [ ] Develop/import synthetic pulse injector
- [ ] Add high-level tests of the full production chain by using real data from https://github.com/legend-exp/legend-testdata @gipert, ...
  - Include all necessary data in legend-testdata
- [x] Let a Python linter run as part of tests to enforce uniform coding conventions (see also #242)
- [x] Add test coverage reporting through https://codecov.io and GitHub bot


Discussion / hints:
test installation doesn't work for me:
```
pygama % pip install .[test]
zsh: no matches found: .[test]
```
Also just running pytests fails:
```
pygama % pytest
ERROR: usage: pytest [options] [file_or_dir] [file_or_dir] [...]
pytest: error: unrecognized arguments: --cov=pygama
  inifile: /Users/jasondet/Work/Coding/pygama/pyproject.toml
  rootdir: /Users/jasondet/Work/Coding/pygama
```
@gipert can you take a look?
This is zsh... you need to wrap `[]` in single quotes, like this: `pip install '.[test]'`.

I've forgotten zsh is now the default shell on OSX.
ah that works! can you update the instructions?

incidentally, using the single quotes also works in bash... maybe make that the instruction?

also, the instructions should specify where to start from (i.e. `.'[test]'` inside the pygama dir, `pygama.'[test]'` from the dir containing pygama) 
What is the plan for unit tests and code coverage for numba-ized functions? (e.g. the processors) We can write unit tests, but because of the JIT compilation, the actual code itself is not counted as covered (see https://github.com/numba/numba/issues/4268).

Should we exclude these functions from the overall coverage? We should still write tests for them of course, but it doesn't seem like we would ever be able to get pytest to mark these blocks of code as "covered".
Good point @slwatkins. What about disabling JIT in unit tests? Would that fix the problem?
It looks like their attempt to solve this in numba has stalled out a bit (no updates to this pull request https://github.com/numba/numba/pull/5660 since 2020). Their recommendation seems to be disabling JIT using the environment variable, but that solution worries me because of the possibility that pure numpy ends up working differently from numba guvectorize. It is possible to access the underlying python function without a decorator by using `__wrapped__`: https://stackoverflow.com/questions/63512189/can-i-run-a-decorated-function-without-a-decorator-functionality (this solution doesn't necessarily work for every kind of decorated function, but it does seem to work for numba):
```
In [1]: from pygama.dsp.processors import trap_filter
In [2]: import numpy as np
In [3]: trap_filter.__wrapped__(np.ones(20), 2, 2, np.zeros(20))
```
We should see if adding a test on the `__wrapped__` function triggers the coverage. Note that the guvectorized ability to call without including the output variable as an argument will not work here (relevant for how the tests are written so far).


It looks like using `__wrapped__` works well, as shown in #399. Right now, it requires the test-writer to effectively duplicate each line of code (we want to test that both the python and the numba versions have the same answers). Is there a more elegant solution?

Perhaps we can write a generic wrapper for a function that calculates the value using the numba and python versions, asserts that they are equal (or up to floating precision), and then returns the value for the test to run?
I just came across the `inspect.unwrap` function, perhaps this would be the way to go instead of using `__wrapped__`?

https://docs.python.org/3/library/inspect.html#inspect.unwrap
I took a stab at something generic for testing numba versus python for the processors, the code is ugly right now (and the variables should probably be renamed), but it seems to work for the numbafied processors that have just the decorator (as opposed to functions like `cusp_filter`)

```python
import re
import inspect
import numpy as np

def numba_vs_python(func, *inputs):

    # get outputs
    all_params = list(inspect.signature(func).parameters)
    output_sizes = re.findall('(\(n*\))', (func.signature.split('->')[-1]))
    noutputs = len(output_sizes)
    output_names = all_params[-noutputs:]

    # numba outputs
    outputs = func(*inputs)
    if noutputs==1:
        outputs = [outputs]

    # unwrapped python outputs
    func_unwrapped = inspect.unwrap(func)
    output_dict = {
        key: np.empty(len(inputs[0])) for key in output_names
    }
    func_unwrapped(*inputs, **output_dict)
    for spec, key in zip(output_sizes, output_dict):
        if spec == '()':
            output_dict[key] = output_dict[key][0]
    output_vals = [output_dict[key] for key in output_names]

    # assert that numba and purepython should be the same (maybe use np.isclose)
    assert all(all(np.atleast_1d(o1) == np.atleast_1d(o2)) for o1, o2 in zip(output_vals, outputs))

    # return value for comparison with expected solution
    return outputs[0] if noutputs == 1 else outputs
```
Thumbs up! Can you open a PR with this? Maybe we merge #399 first.
I'll clean it up a little bit, and open a PR - I can mark #399 as ready, and maybe we can update that test when we're happy with how we're testing these numba functions?
Agreed.

Fix patch:
diff --git a/docs/source/developer.rst b/docs/source/developer.rst
index 649f76f9c..d69f7c9e6 100644
--- a/docs/source/developer.rst
+++ b/docs/source/developer.rst
@@ -5,11 +5,15 @@ The following rules and conventions have been established for the package
 development and are enforced throughout the entire code base. Merge requests
 that do not comply to the following directives will be rejected.
 
-To start developing :mod:`pygama`, clone the remote repository:
+To start developing :mod:`pygama`, fork the remote repository to your personal
+GitHub account (see `About Forks <https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/about-forks>`_).
+If you have not set up your ssh keys on the computer you will be working on,
+please follow `GitHub's instructions <https://docs.github.com/en/authentication/connecting-to-github-with-ssh/generating-a-new-ssh-key-and-adding-it-to-the-ssh-agent>`_. Once you have your own fork, you can clone it via
+(replace "yourusername" with your GitHub username):
 
 .. code-block:: console
 
-  $ git clone https://github.com/legend-exp/pygama
+  $ git clone git@github.com:yourusername/pygama.git
 
 All extra tools needed to develop :mod:`pygama` are listed as optional
 dependencies and can be installed via pip by running:
@@ -121,6 +125,74 @@ Testing
 
     $ pytest --cov=pygama
 
+Testing Numba-Wrapped Functions
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+When using Numba to vectorize Python functions, the Python version of the function
+does not, by default, get directly tested, but the Numba version instead. In
+this case, we need to unwrap the Numba function and test the pure Python version.
+With various processors in :mod:`pygama.dsp.processors`, this means that testing
+and triggering the code coverage requires this unwrapping.
+
+Within the testing suite, we use the :func:`@pytest.fixture()<pytest.fixture>`
+decorator to include a helper function called ``compare_numba_vs_python`` that
+can be used in any test. This function runs both the Numba and pure Python versions
+of a function, asserts that they are equal up to floating precision, and returns the
+output value.
+
+As an example, we show a snippet from the test for
+:func:`pygama.dsp.processors.fixed_time_pickoff`, a processor which uses the
+:func:`@numba.guvectorize()<numba.guvectorize>` decorator.
+
+.. code-block:: python
+
+    def test_fixed_time_pickoff(compare_numba_vs_python):
+        """Testing function for the fixed_time_pickoff processor."""
+
+        len_wf = 20
+
+        # test for nan if w_in has a nan
+        w_in = np.ones(len_wf)
+        w_in[4] = np.nan
+        assert np.isnan(compare_numba_vs_python(fixed_time_pickoff, w_in, 1, ord("i")))
+
+In the assertion that the output is what we expect, we use
+``compare_numba_vs_python(fixed_time_pickoff, w_in, 1, ord("i"))`` in place of
+``fixed_time_pickoff(w_in, 1, ord("i"))``. In general, the replacement to make is
+``func(*inputs)`` becomes ``compare_numba_vs_python(func, *inputs)``.
+
+Note, that in cases of testing for the raising of errors, it is recommended
+to instead run the function twice: once with the Numba version, and once using the
+:func:`inspect.unwrap` function. We again show a snippet from the test for
+:func:`pygama.dsp.processors.fixed_time_pickoff` below. We include the various
+required imports in the snippet for verbosity.
+
+.. code-block:: python
+
+    import inspect
+
+    import numpy as np
+    import pytest
+
+    from pygama.dsp.errors import DSPFatal
+    from pygama.dsp.processors import fixed_time_pickoff
+
+    def test_fixed_time_pickoff(compare_numba_vs_python):
+    "skipping parts of function..."
+    # test for DSPFatal errors being raised
+    # noninteger t_in with integer interpolation
+    with pytest.raises(DSPFatal):
+        w_in = np.ones(len_wf)
+        fixed_time_pickoff(w_in, 1.5, ord("i"))
+
+    with pytest.raises(DSPFatal):
+        a_out = np.empty(len_wf)
+        inspect.unwrap(fixed_time_pickoff)(w_in, 1.5, ord("i"), a_out)
+
+In this case, the general idea is to use :func:`pytest.raises` twice, once with
+``func(*inputs)``, and again with ``inspect.unwrap(func)(*inputs)``.
+
+
 Documentation
 -------------
 


Test patch:
diff --git a/tests/conftest.py b/tests/conftest.py
index 62f8c4400..99c7e57b9 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -1,6 +1,9 @@
+import inspect
 import os
+import re
 from pathlib import Path
 
+import numpy as np
 import pytest
 from legend_testdata import LegendTestData
 
@@ -77,3 +80,54 @@ def dsp_test_file_spm(multich_raw_file):
     assert os.path.exists(out_file)
 
     return out_file
+
+
+@pytest.fixture(scope="session")
+def compare_numba_vs_python():
+    def numba_vs_python(func, *inputs):
+        """
+        Function for testing that the numba and python versions of a
+        function are equal.
+
+        Parameters
+        ----------
+        func
+            The Numba-wrapped function to be tested
+        *inputs
+            The various inputs to be passed to the function to be
+            tested.
+
+        Returns
+        -------
+        func_output
+            The output of the function to be used in a unit test.
+
+        """
+        # parse outputs from function signature
+        all_params = list(inspect.signature(func).parameters)
+        output_sizes = re.findall(r"(\(n*\))", func.signature.split("->")[-1])
+        noutputs = len(output_sizes)
+        output_names = all_params[-noutputs:]
+
+        # numba outputs
+        outputs_numba = func(*inputs)
+        if noutputs == 1:
+            outputs_numba = [outputs_numba]
+
+        # unwrapped python outputs
+        func_unwrapped = inspect.unwrap(func)
+        output_dict = {key: np.empty(len(inputs[0])) for key in output_names}
+        func_unwrapped(*inputs, **output_dict)
+        for spec, key in zip(output_sizes, output_dict):
+            if spec == "()":
+                output_dict[key] = output_dict[key][0]
+        outputs_python = [output_dict[key] for key in output_names]
+
+        # assert that numba and python are the same up to floating point
+        # precision, setting nans to be equal
+        assert np.allclose(outputs_numba, outputs_python, equal_nan=True)
+
+        # return value for comparison with expected solution
+        return outputs_numba[0] if noutputs == 1 else outputs_numba
+
+    return numba_vs_python
diff --git a/tests/dsp/processors/test_fixed_time_pickoff.py b/tests/dsp/processors/test_fixed_time_pickoff.py
index 0a1bb522c..b2634f4fe 100644
--- a/tests/dsp/processors/test_fixed_time_pickoff.py
+++ b/tests/dsp/processors/test_fixed_time_pickoff.py
@@ -1,3 +1,5 @@
+import inspect
+
 import numpy as np
 import pytest
 
@@ -5,7 +7,7 @@
 from pygama.dsp.processors import fixed_time_pickoff
 
 
-def test_fixed_time_pickoff():
+def test_fixed_time_pickoff(compare_numba_vs_python):
     """Testing function for the fixed_time_pickoff processor."""
 
     len_wf = 20
@@ -13,72 +15,46 @@ def test_fixed_time_pickoff():
     # test for nan if w_in has a nan
     w_in = np.ones(len_wf)
     w_in[4] = np.nan
-    assert np.isnan(fixed_time_pickoff(w_in, 1, ord("i")))
-
-    a_out = np.empty(len_wf)
-    fixed_time_pickoff.__wrapped__(w_in, 1, ord("i"), a_out)
-    assert np.isnan(a_out[0])
+    assert np.isnan(compare_numba_vs_python(fixed_time_pickoff, w_in, 1, ord("i")))
 
     # test for nan if nan is passed to t_in
     w_in = np.ones(len_wf)
-    assert np.isnan(fixed_time_pickoff(w_in, np.nan, ord("i")))
-
-    a_out = np.empty(len_wf)
-    fixed_time_pickoff.__wrapped__(w_in, np.nan, ord("i"), a_out)
-    assert np.isnan(a_out[0])
+    assert np.isnan(compare_numba_vs_python(fixed_time_pickoff, w_in, np.nan, ord("i")))
 
     # test for nan if t_in is negative
     w_in = np.ones(len_wf)
-    assert np.isnan(fixed_time_pickoff(w_in, -1, ord("i")))
-
-    a_out = np.empty(len_wf)
-    fixed_time_pickoff.__wrapped__(w_in, -1, ord("i"), a_out)
-    assert np.isnan(a_out[0])
+    assert np.isnan(compare_numba_vs_python(fixed_time_pickoff, w_in, -1, ord("i")))
 
     # test for nan if t_in is too large
     w_in = np.ones(len_wf)
-    assert np.isnan(fixed_time_pickoff(w_in, len_wf, ord("i")))
-
-    a_out = np.empty(len_wf)
-    fixed_time_pickoff.__wrapped__(w_in, len_wf, ord("i"), a_out)
-    assert np.isnan(a_out[0])
+    assert np.isnan(compare_numba_vs_python(fixed_time_pickoff, w_in, len_wf, ord("i")))
 
     # test for DSPFatal errors being raised
     # noninteger t_in with integer interpolation
     with pytest.raises(DSPFatal):
         w_in = np.ones(len_wf)
         fixed_time_pickoff(w_in, 1.5, ord("i"))
-
     with pytest.raises(DSPFatal):
         a_out = np.empty(len_wf)
-        fixed_time_pickoff.__wrapped__(w_in, 1.5, ord("i"), a_out)
+        inspect.unwrap(fixed_time_pickoff)(w_in, 1.5, ord("i"), a_out)
 
     # unsupported mode_in character
     with pytest.raises(DSPFatal):
         w_in = np.ones(len_wf)
         fixed_time_pickoff(w_in, 1.5, ord(" "))
-
     with pytest.raises(DSPFatal):
         a_out = np.empty(len_wf)
-        fixed_time_pickoff.__wrapped__(w_in, 1.5, ord(" "), a_out)
+        inspect.unwrap(fixed_time_pickoff)(w_in, 1.5, ord(" "), a_out)
 
     # linear tests
     w_in = np.arange(len_wf, dtype=float)
-    assert fixed_time_pickoff(w_in, 3, ord("i")) == 3
-
-    a_out = np.empty(len_wf)
-    fixed_time_pickoff.__wrapped__(w_in, 3, ord("i"), a_out)
-    assert a_out[0] == 3
+    assert compare_numba_vs_python(fixed_time_pickoff, w_in, 3, ord("i")) == 3
 
     chars = ["n", "f", "c", "l", "h", "s"]
     sols = [4, 3, 4, 3.5, 3.5, 3.5]
 
     for char, sol in zip(chars, sols):
-        assert fixed_time_pickoff(w_in, 3.5, ord(char)) == sol
-
-        a_out = np.empty(len_wf)
-        fixed_time_pickoff.__wrapped__(w_in, 3.5, ord(char), a_out)
-        assert a_out[0] == sol
+        assert compare_numba_vs_python(fixed_time_pickoff, w_in, 3.5, ord(char)) == sol
 
     # sine wave tests
     w_in = np.sin(np.arange(len_wf))
@@ -94,11 +70,9 @@ def test_fixed_time_pickoff():
     ]
 
     for char, sol in zip(chars, sols):
-        assert np.isclose(fixed_time_pickoff(w_in, 3.25, ord(char)), sol)
-
-        a_out = np.empty(len_wf)
-        fixed_time_pickoff.__wrapped__(w_in, 3.25, ord(char), a_out)
-        assert np.isclose(a_out[0], sol)
+        assert np.isclose(
+            compare_numba_vs_python(fixed_time_pickoff, w_in, 3.25, ord(char)), sol
+        )
 
     # last few corner cases of 'h'
     w_in = np.sin(np.arange(len_wf))
@@ -109,8 +83,6 @@ def test_fixed_time_pickoff():
     ]
 
     for ftp, sol in zip(ftps, sols):
-        assert np.isclose(fixed_time_pickoff(w_in, ftp, ord("h")), sol)
-
-        a_out = np.empty(len_wf)
-        fixed_time_pickoff.__wrapped__(w_in, ftp, ord("h"), a_out)
-        assert np.isclose(a_out[0], sol)
+        assert np.isclose(
+            compare_numba_vs_python(fixed_time_pickoff, w_in, ftp, ord("h")), sol
+        )