Skip to content

Fix use_dual type hints#1612

Open
Sanjays2402 wants to merge 1 commit into
UXARRAY:mainfrom
Sanjays2402:fix/use-dual-type-hints
Open

Fix use_dual type hints#1612
Sanjays2402 wants to merge 1 commit into
UXARRAY:mainfrom
Sanjays2402:fix/use-dual-type-hints

Conversation

@Sanjays2402

@Sanjays2402 Sanjays2402 commented Jul 23, 2026

Copy link
Copy Markdown

Fixes part of #1593 but does not fully close it.

Overview

  • Narrow use_dual to bool across the public grid-loading APIs and Grid.from_dataset, matching the documented and implemented contract.
  • Add a focused type-hint regression test covering all four signatures.

PR Checklist

General

  • An issue is linked created and linked
  • Add appropriate labels
  • Filled out Overview and Expected Usage (if applicable) sections

Testing

  • Adequate tests are created if there is new functionality
  • Tests cover all possible logical paths in your function
  • Tests are not too basic (such as simply calling a function and nothing else)

Documentation

  • [N/A] Docstrings have been added to all new functions
  • [N/A] Docstrings have updated with any function changes
  • [N/A] Internal functions have a preceding underscore (_) and have been added to docs/internal_api/index.rst
  • [N/A] User functions have been added to docs/user_api/index.rst

Examples

  • [N/A] Any new notebook examples added to docs/examples/ folder
  • [N/A] Clear the output of all cells before committing
  • [N/A] New notebook files added to docs/examples.rst toctree
  • [N/A] New notebook files added to new entry in docs/gallery.yml with appropriate thumbnail photo in docs/_static/thumbnails/

The public grid-loading APIs document and implement use_dual as a boolean, but their annotations also accepted None. Align the public and internal signatures with the actual contract and add focused regression coverage.
@Sevans711

Copy link
Copy Markdown
Collaborator

Thank you for these contributions, @Sanjays2402! This looks like a clean fix to the None type hints on the use_dual kwarg, and one good way to test things.

I have a few thoughts/notes before we get this merged into main:

  1. Note: this doesn't actually solve the entirety of Type hints allows None for too many parameters #1593, which only used use_dual as an example. In reality, there are many more cases of kwargs which type hint None despite not really handling None internally. Examples include: destination in UxDataArray.difference(); start_index in Grid.from_topology(); and tol in Grid.from_structured(). This is not a complete list of examples where this needs to be solved (it will take more time to generate the complete list). Because of this, I am going to change the "Closes #XXX" phrase to "Fixes part of #XXX but does not fully close it" so that the issue can remain open even after this PR merges.
  2. Asking other developers (e.g. @erogluorhan, @rajeeja) do we want this sort of test case for type hinting in general? This test works but looks dependent on type hint spelling, and I'm not sure if it would be ideal to set the expectation of having similar tests for all the other kwargs with this issue and the functions in which they appear. Options include: (A) merge as-is and expect similar tests for all other such kwargs, (B) keep this test but add a comment in the code clarifying it is a "spot check" and not expected for all such kwargs, (C) drop the test and do not worry about testing type hinting, (D) add more comprehensive type hinting checks (mypy/pyright?) to the CI.

Also, out of curiosity, @Sanjays2402 what led you to uxarray? It is always helpful to hear how people find out about the project!

@Sevans711 Sevans711 added the bug Something isn't working label Jul 23, 2026
@Sevans711
Sevans711 self-requested a review July 23, 2026 16:24
@cmdupuis3

Copy link
Copy Markdown
Collaborator

Yeah, I've been noticing this too, seems like an anti-pattern to allow both None and false to mean the same thing. Maybe it's a legacy API issue?

@Sanjays2402

Copy link
Copy Markdown
Author

Agreed on the scope, "fixes part of #1593" is the accurate framing, use_dual was just the example that surfaced it.

On the test: I'd lean (B). It's spot-check by nature and I share your concern about setting an expectation that every kwarg gets a hand-written type-hint test, that gets unmaintainable fast and the test does key off hint spelling. Happy to add a comment saying as much. If the team would rather go (C) I won't argue, the signature fix stands on its own without it.

Say which way you want it and I'll push.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants