[Sevans711]

Closes #1738

Overview

Issue #1738 was apparently already solved by PR #1775, though this wasn't realized at the time. E.g., running the MCVE from that issue report currently succeeds on main. It succeeds when checking out to the commit which merged 1775 to main, and it fails when checking out to the commit just before 1775 was merged. The PR here adds test_scrip_load_is_deterministic as a regression test to confirm the bug initially reported in 1738 is actually solved.

Discussion in thread of 1775 (see e.g. #1775 (comment)) also scoped 1738 to additionally check whether the dask and eager paths produce the same ordering as each other, and possibly make changes if the orderings disagreed. A close investigation across a few examples shows that these do indeed already produce the same ordering; this just wasn't checked directly in 1775. So, the PR here also updates test_scrip_dask_lazy_dedup_matches_eager and test_scrip_dask_lazy_dedup_matches_eager_radians to assert exact equality for grid_lazy == grid_eager, instead of comparing after "normalizing" the order of each based on the winding order from face_node_connectivity. The orders are already exactly the same in both paths, before normalizing.

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

• [N/A] Docstrings updated with any function changes, and included in all new functions
• [N/A] 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 / Claude Sonnet 5.5, to help with understanding: the problem, which parts of the commits from #1775 solved it, whether the existing tests demonstrated it was solved (they did not), and which tests to consider updating/adding.

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

[cmdupuis3]

1. Grid.__eq__ can't tell when only one of the coordinates is wrong. In grid.py line 814 it should be and instead of or. This isn't new in this PR but the changes here are the first to actually rely on it.

2. The test doesn't actually test the part that would trigger nondeterminism, because chunks="auto" leaves the data as a single dask block.

[Sevans711]

1. Grid.__eq__ can't tell when only one of the coordinates is wrong. In grid.py line 814 it should be and instead of or. This isn't new in this PR but the changes here are the first to actually rely on it.

Wow, great point… I opened #1881 to improve visibility for this bug. My preference would be to solve it via a separate PR. Since the PR here is just adding tests I think it's fine to block this one until that issue gets fixed.

2. The test doesn't actually test the part that would trigger nondeterminism, because chunks="auto" leaves the data as a single dask block.

I'm pretty confident that the kw==dict() case hits the nondeterminism (the test loops across for kw in (dict(), dict(chunks="auto"))). Locally I confirmed (see original PR description message) that the tests do indeed fail when run against the version of main just before #1775 merged.

[rajeeja]

Agree with @cmdupuis3 that #1881 should land first to fix the or -> and in Grid.__eq__.

As you both noted, one small follow-up here: could we update test_scrip_load_is_deterministic to include a guaranteed multi-partition chunks value, e.g. chunks={"grid_size": 7}? chunks="auto" can be single-block and won't exercise the dask dedup path. Thanks for adding the regression test for #1738!

[Sevans711]

As you both noted, one small follow-up here: could we update test_scrip_load_is_deterministic to include a guaranteed multi-partition chunks value, e.g. chunks={"grid_size": 7}? chunks="auto" can be single-block and won't exercise the dask dedup path.

Sure, I've added a check for chunks={"grid_size": n_face//4} in here too. (Using a fixed value of 7 turned out to be very slow for the example with 21600 faces.) For future reference, in the commit on main just before the underlying issue was fixed, the test failed when using this value for chunks failed, and also when using chunks="auto". So, now I'm pretty confident that the nondeterminism was actually always happening previously, regardless of chunks.

I'll re-request reviews here once #1881 gets fixed!

[Sevans711]

I believe this is ready for re-reviews now that #1889 has merged!