Apply function to points within circular neighborhood - #941
Conversation
Kept neighborhood and dual additions
|
HI @ahijevyc Apologies for not getting to this PR earlier. Looking at the implementation here, it looks great. It does however bring to light a possible need for us to consider a better, more streamlined, approach to handling these types of groupings and then applying some function on the result. I mention this because of our Topological Aggregations. For this family of functions, we have distinct methods (i.e. Very generally speaking, these functions essentially:
I wonder if this would be a good opportunity to extend the inherited I'm not sure of calling these approaches "kernels" is appropriate, but for the sake of this example, we could provide spatial kernels the user could pass into # radial neighborhood of r=0.25
uxds['t2m'].groupby(kernel=ux.BoundingCircle(r=0.25)).mean()
# 2 deg by 2 deg bounding box
uxds['t2m'].groupby(kernel=ux.BoundingBox(dlon=2, dlat=2))
# group the nodes that surround each face and find the maximum
uxds['node_centered_var'].groupby(kernel=ux.FaceNode()).max()
# this is equivalent to the following in the current release
uxds['node_centered_var'].topological_max(destination='face')I'll ping @aaronzedwick and @erogluorhan for their thoughts on this. I personally really like the design above and think that it aligns well with the overall design. |
That is interesting. You suggesting changing the way we do aggregations entirely? Then this would affect the reduction PR I am working on then. Perhaps this PR could implement that change if you wish. I am fine with this, if you want to, it sounds like it would be intuitive. |
We should be changing the API for it, since the current one is not sustainable if we plan to implement many times of spatial grouping methods. The underlying functionality would remain mostly unchanged.
If we do decide to do this, I'll open up a separate PR starting with the topological aggregations. |
So would the reductions PR be obsolete? |
No. The underlying implementation would remain the same, since we would still need those implemented. This would just provide a different interface for it, with a more "Xarray-like" interface. |
Ah, okay, I see. That makes sense, thanks for the clarification! |
|
pre-commit.ci autofix |
Renames the neighborhood classes and accessors to the singular `Neighborhood`, `DataArrayNeighborhood`, and `DatasetNeighborhood`. The plural read as a list or array of neighborhoods rather than one object describing the neighborhood of every element, which would have been confusing as soon as anything held several of them. Adds `_BoundNeighborhoodReductions`, an abstract base carrying the eleven reductions once for both data-bound classes. Each method names the `Neighborhood` reduction it stands for and hands it to `_map`, which subclasses implement to say which data it runs on -- the only thing that differs between a neighborhood bound to one variable and one bound to a whole dataset. This drops the eleven method bodies each bound class used to define, and removes the `getattr(neighborhood, method)` string dispatch in `DatasetNeighborhood`, which had reintroduced exactly the lookup table the kernels are documented as not needing. Choosing a kernel and preparing its parameter now happens in `Neighborhood` alone, so a bound reduction cannot reach a different kernel, or a different ddof, than the unbound one it names. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
I have some more changes to consider on my Instead, my API proposal is that we have all the named kernels be methods. So, we can call the vectorized kernels by name without mystifying what's actually running, and have This has the added advantage that the kernels are now separable from the neighborhood construction, so you can store a neighborhood and call multiple kernels on it rather than constructing a new neighborhood each time. On the other hand, it raises the possibility of having multiple neighborhoodsesesssses, so I renamed them to be singular as objects. |
I like this design, it is simpler , less duplication and more pythonic. The whole _filter wasn't really needed. One question - Do we really need _BoundNeighborhoodReductions as an ABC, or can the common reduction logic be expressed through a simpler composition/delegation pattern? |
|
I kind of think there should be a way to unify all three classes somehow, but I haven't found it yet. I can try some more things and let you know. |
`_BoundNeighborhoodReductions` becomes a plain class with a documented
`_map` stub. Neither subclass is ever instantiated without `_map`, and
both live in this module, so `abc` was buying an instantiation-time
error nobody could hit.
Each bound reduction now hands `_map` the `Neighborhood` method itself
rather than a lambda that looks it up:
return self._map(Neighborhood.median)
The reference resolves when the class body runs, so a reduction that
`Neighborhood` does not define cannot be spelled here at all. That
completes a progression: `getattr(nb, "median")` failed at call time,
`lambda nb, uxda: nb.median(uxda)` also failed at call time, and this
fails at import.
The vocabulary is still spelled twice in all, once per signature -- data
taking on `Neighborhood`, data bound here. Collapsing that further would
mean generating the methods, which would break the signatures
`test_invalid_reduction_arguments` pins, and would sit badly beside the
explicit style of the tree classes above.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@rajeeja Alright, I refactored it a bit and got rid of the ABC (although spiritually it still basically is one). I attempted taking a compositional approach, but there's no nice solution that doesn't clutter up the API or duplicate all the reduction methods, or have some other drawbacks. |
ASV BenchmarkingBenchmark Comparison ResultsBenchmarks that have improved:
Benchmarks that have stayed the same:
Benchmarks that have got worse:
|
|
pre-commit.ci autofix |
cmdupuis3
left a comment
cmdupuis3
left a comment
There was a problem hiding this comment.
Approving, but a good amount now has my fingers on it, so I'd like to defer for another approval.
6 participants
Apply a neighborhood filter within a circular radius r to a UxDataset or UxDataArray.
Closes #930
Overview
This is kind of like
uxarray.UxDataArray.inverse_distance_weighted_remap, but the neighborhood is defined by distance, not a number of nearest neighbors. This is ideally suited for a variable resolution mesh, in which a constant of neighbors doesn't have a constant sized neighborhood. Another difference is that this neighborhood filter does not weight data by inverse distance.Just like
uxarray.UxDataArray.subset.bounding_circlethis function usesball_tree.query_radiusto select grid elements in a circular neighborhood, but this function finds the neighborhood for all elements in grid, not just one center_coordinate.The filter function
funcmay be a user-defined function, but usesnp.meanby default. It could bemin,max,np.median. It can even use functions that require additional arguments, likenp.percentileif you supply the argument(s) withfunctools.partial(see below)Expected Usage
PR Checklist
General
Testing
Documentation
_);_neighborhood_filteris internal touxarray/grid/neighbors.pydocs/api.rst(the split user/internal api files no longer exist)Examples
docs/examples/folderdocs/examples.rsttoctreedocs/gallery.ymlwith appropriate thumbnail photo indocs/_static/thumbnails/