Fixes uxarray.concat bugs and adds docstring - #1719
Open
Sevans711 wants to merge 4 commits into
Open
Conversation
adds clearer error when len(objs)==0, and allows iterable objs without len(), by converting to tuple at start of function.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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):
uxarray.concat. Updates the function signature to clarify thatdimis a required parameter (instead of hiding it inside*args).==instead ofis(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).Minor expansions of scope:
self is otherinGrid.__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 ofis, 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 otheris blazingly fast compared to array comparison).import uxarray.core.datasetin 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.remap.utils._to_datasetinstead of brittle namespace population behavior (it previously assumeduxarray.core.dataarrayexisted after callingimport uxarray.core.dataset, but without ever explicitly importinguxarray.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 afterdimmust 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 withconcat, lack of appearance in any documentation examples, and complete lack of a docstring, lead me to believeconcatwas not being used much (if at all) before this.Misc notes:
import uxarrayperformance, because UxDataset is already being imported in api.py, and the dataset.py file already imports UxDataArray.PR Checklist
General
Testing & Benchmarking
Documentation and Examples
docs/api.rst; internal (private) function names start with an underscore (_)AI Disclosure
AI Usage: GitHub Copilot's inline code suggestions, and a brief Claude chat about docstring style.