Repository: lincc-frameworks/nested-pandas

Issue:
Make it harder to pass a wrong column name to NestedFrame.reduce
**Feature request**

One of the bugs I make in my code all the time is passing a wrong column name to `NestedFrame.reduce(func, *args)`. This makes it a "constant" argument, instead of a variable value/array from the actual data frame. Sometimes this type of bug is hard to debug, for example when array of strings is expected (think about "lc.band").

It would be nice to find a way to make it harder to make such a mistake.

I see couple of options here:
- Disallow all positional "constant" arguments: people may use `**kwargs` or `lambda`/`partial` if they must use `*args`
- Disallow string positional arguments: more flexible, but may be a little bit more confusing for users (different behavior for different types of arguments)
- Print a warning if a positional argument string doesn't match to any column, with a hint: string value "Xx" is passed, maybe you meant column "XX"?

**Before submitting**
Please check the following:

- [x] I have described the purpose of the suggested change, specifying what I need the enhancement to accomplish, i.e. what problem it solves.
- [x] I have included any relevant links, screenshots, environment information, and data relevant to implementing the requested feature, as well as pseudocode for how I want to access the new functionality.
- [x] If I have ideas for how the new feature could be implemented, I have provided explanations and/or pseudocode and/or task lists for the steps.

Discussion:
@dougbrn: Thanks for opening this, between this and our latest conversations at LINCC-up there are some opportunities to change the API of reduce quite a bit. Not sure if I have a preference between the options you have already laid out, but will add some alternatives:

* Regress back to an apply-like interface, encourage more lambda style operations
* Make arg/kwarg containers for func params? E.g.
```
nf.reduce(my_func, func_params = {"columns_arg": ["a", "b"], "constant_kwarg":3})
```

Maybe it would be better to try to limit the positional args in favor of labeled kwargs as you say in the first option.@hombit: ~An option somewhere between what we have now and what you propose:~

```python
nf.reduce(my_func, ["col1", "lc.col2"], *udf_args, **udf_kwargs, infer_nesting=True)
```

Update: it will not work. I think I still want to just remove `*udf_args` from the signature

Solution patch:
diff --git a/src/nested_pandas/nestedframe/core.py b/src/nested_pandas/nestedframe/core.py
index 36fed5f..69dcfa9 100644
--- a/src/nested_pandas/nestedframe/core.py
+++ b/src/nested_pandas/nestedframe/core.py
@@ -1007,8 +1007,9 @@ def reduce(self, func, *args, infer_nesting=True, **kwargs) -> NestedFrame:  # t
             columns to apply the function to. See the Notes for recommendations
             on writing func outputs.
         args : positional arguments
-            Positional arguments to pass to the function, the first *args should be the names of the
-            columns to apply the function to.
+            A list of string column names to pull from the NestedFrame to pass along
+            to the function. If the function has additional arguments, pass them as
+            keyword arguments (e.g. `arg_name=value`).
         infer_nesting : bool, default True
             If True, the function will pack output columns into nested
             structures based on column names adhering to a nested naming
@@ -1083,10 +1084,20 @@ def reduce(self, func, *args, infer_nesting=True, **kwargs) -> NestedFrame:  # t
             # Stop when we reach an argument that is not a valid column, as we assume
             # that the remaining args are extra arguments to the function
             if not isinstance(arg, str):
-                break
+                raise TypeError(
+                    f"Received an argument '{arg}' that is not a string. "
+                    "All arguments to `reduce` must be strings corresponding to"
+                    " column names to pass along to the function. If your function"
+                    " has additional arguments, pass them as kwargs (arg_name=value)."
+                )
             components = self._parse_hierarchical_components(arg)
             if not self._is_known_column(components):
-                break
+                raise ValueError(
+                    f"Received a string argument '{arg}' that was not found in the columns list. "
+                    "All arguments to `reduce` must be strings corresponding to"
+                    " column names to pass along to the function. If your function"
+                    " has additional arguments, pass them as kwargs (arg_name=value)."
+                )
             layer = "base" if len(components) < 2 else components[0]
             col = components[-1]
             requested_columns.append((layer, col))


Test patch:
diff --git a/tests/nested_pandas/nestedframe/test_nestedframe.py b/tests/nested_pandas/nestedframe/test_nestedframe.py
index 5857c40..45f6f1c 100644
--- a/tests/nested_pandas/nestedframe/test_nestedframe.py
+++ b/tests/nested_pandas/nestedframe/test_nestedframe.py
@@ -966,7 +966,7 @@ def offset_avg(offset, col_to_avg, column_names):
         sum([7, 10, 7]) / 3.0,
     ]
 
-    result = nf.reduce(offset_avg, "b", "packed.c", ["offset_avg"])
+    result = nf.reduce(offset_avg, "b", "packed.c", column_names=["offset_avg"])
     assert len(result) == len(nf)
     assert isinstance(result, NestedFrame)
     assert result.index.name == "idx"
@@ -978,7 +978,7 @@ def offset_avg(offset, col_to_avg, column_names):
     def make_id(col1, prefix_str):
         return f"{prefix_str}{col1}"
 
-    result = nf.reduce(make_id, "b", "some_id_")
+    result = nf.reduce(make_id, "b", prefix_str="some_id_")
     assert result[0][1] == "some_id_4"
 
 
@@ -1095,6 +1095,27 @@ def complex_output(flux):
     assert list(result.lc.nest.fields) == ["flux_quantiles", "labels"]
 
 
+def test_reduce_arg_errors():
+    """Test that reduce errors based on non-column args trigger as expected"""
+
+    ndf = generate_data(10, 10, seed=1)
+
+    def func(a, flux, add):
+        """a function that takes a scalar, a column, and a boolean"""
+        if add:
+            return {"nested2.flux": flux + a}
+        return {"nested2.flux": flux + a}
+
+    with pytest.raises(TypeError):
+        ndf.reduce(func, "a", "nested.flux", True)
+
+    with pytest.raises(ValueError):
+        ndf.reduce(func, "ab", "nested.flux", add=True)
+
+    # this should work
+    ndf.reduce(func, "a", "nested.flux", add=True)
+
+
 def test_scientific_notation():
     """
     Test that NestedFrame.query handles constants that are written in scientific notation.