Reduce imports inside functions#1603
Merged
Merged
Conversation
Sevans711
added
the
developer experience
erogluorhan
reviewed
Jul 21, 2026
erogluorhan
left a comment
erogluorhan
left a comment
Member
There was a problem hiding this comment.
The work here seems great to me! Please make sure all the tests are passing. I will get back to your discussion points and approve this then
dylannelson
approved these changes
Jul 21, 2026
dylannelson
left a comment
dylannelson
left a comment
Collaborator
There was a problem hiding this comment.
The notes and comments here helped a lot, appreciate it! Thanks!
ASV BenchmarkingBenchmark Comparison ResultsBenchmarks that have improved:
Benchmarks that have stayed the same:
|
Collaborator
Author
|
Thank you both for taking a look! Re: @erogluorhan's point about tests:
All the tests do seem to pass with the code here, I manually triggered a CI run on this branch (https://github.com/UXARRAY/uxarray/actions/runs/29865868247), after the original run failed one test just due to a network error (link for posterity: https://github.com/UXARRAY/uxarray/actions/runs/29864267121/job/88748409432?pr=1603). This rerun involved no code changes. I also opened #1604 for further discussion about network failures in the CI autotests. What is the preferred workflow when this happens? I wasn't able to find a "rerun tests directly in the PR interface" button. One option: I can push a commit with basically no changes (e.g., comment at the single top-level scipy import to say "maybe this should move to be not at top-level? see PR # 1603 for more details"). I will wait for your response before pushing that though, to help the current issue remain clear until then. |
Collaborator
Author
|
Tests now all passed successfully, thank you for showing me where the "rerun failed jobs in this PR" button lives, @erogluorhan! I suspect I may have failed to find it before due to looking too early, before all the jobs had finished? Or, maybe I simply didn't know where to look. I left this comment about it: #1604 (comment) for future reference. |
erogluorhan
self-requested a review
July 22, 2026 21:38
erogluorhan
approved these changes
Jul 22, 2026
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
3 participants
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 #1575
Overview
There are some places throughout the uxarray codebase where import is being used inside functions. This PR targets imports which are not "heavy" or which are already being imported non-lazily (i.e., at they are imported at "top-level" / top of file), changing them to be consistently imported non-lazily everywhere in uxarray.
Notes about all packages being imported internally in at least one location (e.g. imported inside a function; not just at top of file):
antimeridian: not currently at top-level anywhere. Tiny required dependency, seems trivially-lightweight to import. But, might become optional some day...? → keep internal.dask: not currently at top-level anywhere, and not trivially-lightweight to import → keep internaldatetime: trivially lightweight and builtin, no need to import lazily → move to top.IPython: not currently at top-level anywhere; optional (but forgot to list in pyproject.toml at all), and will probably be removed eventually, whenever upstream fix is released (see discussion in Restore matplotlib backend after HoloViews matplotlib plot #1538 and upstream issue at hv.extension('matplotlib') clobbers active Matplotlib backend and IPython inline display integration holoviz/holoviews#6955) → do not touch in this PR.numpy: essential for core functionality and definitions, already at top level in many places → move to top level.pyfma: only appears in an otherwise-unused module? (See uxarray/utils/computing.py not used anywhere, andpyfmanot in pyproject.toml #1602) → do not touch in this PR.scipy: non-trivial import size,but does exist as top-level import inEDIT: this would be fully addressed by Move imports for should-be-optional dependencies #1606, which moves that scipy import to be internal instead → do not touch in this PR.weights.py, which makes this a bit ambiguous. → did not touch here, but open for further discussion (should the scipy import in weights.py be moved to be internal instead?).sklearn: not currently at top-level anywhere; non-trivial import size → keep internal.shapely: same story as withantimeridian: not currently at top-level anywhere. Tiny required dependency, seems trivially-lightweight to import. But, might become optional some day...? → keep internal.warnings: trivially lightweight and builtin, no need to import lazily → move to top.xarray: essential for core functionality and definitions, already at top level in many places → move to top level.cartopygeopandasholoviewshvplotmatplotlibspatialpandasNotes about importing from uxarray internally to functions:
from uxarray.core.dataarray import UxDataArraymust be kept inside functions, to avoid circular imports.UxDataArray.iselcontainingfrom uxarray.core.utils import _validate_indexers, where uxarray methods are imported internally without a clear compelling reason to do so. These should probably be moved to top-level. However, the original scope of issue Reduce number of imports inside functions #1575 did not clarify this possibility, and uxarray's explicit non-relative imports make me think it could be an intentional design choice, to reduce "cascading" imports. I think it makes more sense to open a separate issue to discuss internal imports like "from uxarray... import ...", instead of trying to also consider that here. If PR reviewers agree, let's open that issue before this PR gets mergedMisc notes:
PR Checklist
General