Skip to content

Fixes uxarray.concat bugs and adds docstring - #1719

Open
Sevans711 wants to merge 4 commits into
mainfrom
sevans/fix-concat
Open

Fixes uxarray.concat bugs and adds docstring#1719
Sevans711 wants to merge 4 commits into
mainfrom
sevans/fix-concat

Conversation

@Sevans711

Copy link
Copy Markdown
Collaborator

Closes #1642

Overview

Addresses all bugs mentioned in #1642. This PR does the following (with numbers here corresponding to numbers in the original issue report):

  1. Adds a clear docstring to uxarray.concat. Updates the function signature to clarify that dim is a required parameter (instead of hiding it inside *args).
  2. Uses explicit type checking to ensure objects are either all UxDataArrays or all UxDatasets.
  3. Compares uxgrids using == instead of is (well, technically, uses !=, but the important part is that the thing being compared is whether the grids are equal, not whether they refer to literally the same object).
  4. Now supports concatenating UxDataArray objects, not just UxDatasets.
  5. Adds regression tests for (1), (2), and (3), and a few tiny checks of correctness.

Minor expansions of scope:

  • Adds a check for self is other in Grid.__eq__ to possibly speed up comparison in a relatively common case. Spot checks currently seem to show negligible performance improvements for pre-existing code (e.g., in calculus operations like curl() which compare uxgrids). Might become relevant to performance in concat, now that it compares grids using == instead of is, or possibly in the future when fixing issue Math with multiple uxarray objects with different grids silently succeeds, when data dims are otherwise compatible #1718. Even if impact on performance remains negligible in all normal use-cases, it definitely does not hurt to add this check, as it has negligibly tiny costs (self is other is blazingly fast compared to array comparison).
  • Adds type hinting for UxDataArray.to_xarray() output type (xr.DataArray).
  • Moves top-level import uxarray.core.dataset in uxarray/remap/utils.py to be inside the relevant function instead. It was causing a circular import error when adding explicit import of UxDataArray into api.py (see notes below). This solution feels like a better style than preventing UxDataArray import in api.py; I don't think it makes sense for internal utils files to eagerly import (at top of file) from such a core class file like dataset.
    • Also, uses explicit import of UxDataArray in remap.utils._to_dataset instead of brittle namespace population behavior (it previously assumed uxarray.core.dataarray existed after calling import uxarray.core.dataset, but without ever explicitly importing uxarray.core.dataarray).
  • concat() now utilizes keyword-only arguments after the second possibly-positional argument (i.e. concat(objs, dim, third_arg) is no longer allowed, everything after dim must be passed as keyword argument); see Utilize keyword-only arguments, at least in public API #1573 for motivation. Normally I might hesitate to remove the ability to pass positional args without any deprecation cycle, however the numerous bugs with concat, lack of appearance in any documentation examples, and complete lack of a docstring, lead me to believe concat was not being used much (if at all) before this.

Misc notes:

  • The new import of UxDataArray in api.py cannot degrade import uxarray performance, because UxDataset is already being imported in api.py, and the dataset.py file already imports UxDataArray.
  • Similarly, the new import of pandas in api.py cannot degrade performance, because the dataarray.py file imports UxDataArrayPlotAccessor, and the plot/accessor.py file already imports pandas.
  • Maybe it could be nice to support concat() with a mix of xarray and uxarray objects eventually? Though, users could also just turn them all into uxarray objects first, so that would probably not be a high priority.

PR Checklist

General

  • An issue is created and linked
  • Added appropriate labels (if your uxarray repo permissions allow it)
  • Filled out Overview and Expected Usage (if applicable) sections

Testing & Benchmarking

  • There is adequate test coverage of changes from this PR (add new tests if needed)
  • [N/A] If this PR could affect performance, ran ASV benchmarks and confirmed they show expected behavior (add a new benchmark if necessary)

Documentation and Examples

  • Docstrings updated with any function changes, and included in all new functions
  • User (public) functions added to docs/api.rst; internal (private) function names start with an underscore (_)
  • [N/A] If touched any notebook files, cleared the output of all cells before committing
  • [N/A] If added new notebook files, put into appropriate directories and referenced in appropriate files

AI Disclosure

AI Usage: GitHub Copilot's inline code suggestions, and a brief Claude chat about docstring style.

  • I have tested and take responsibility for all AI-generated content in my PR.

adds clearer error when len(objs)==0, and allows iterable objs without len(), by converting to tuple at start of function.
@Sevans711 Sevans711 self-assigned this Aug 28, 2026
@Sevans711 Sevans711 added bug Something isn't working documentation Improvements or additions to documentation labels Aug 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

uxarray.concat implementation incomplete/buggy and needs a docstring

1 participant