Skip to content

Run Narwhals tests on a single worker and fix issues#923

Merged
scott-routledge2 merged 10 commits intomainfrom
scott/narwhals_single_rank
Nov 13, 2025
Merged

Run Narwhals tests on a single worker and fix issues#923
scott-routledge2 merged 10 commits intomainfrom
scott/narwhals_single_rank

Conversation

@scott-routledge2
Copy link
Contributor

@scott-routledge2 scott-routledge2 commented Nov 11, 2025

Changes included in this PR

Some tests like those in unique_test.py have non-deterministic results that are both correct, to avoid too many testing fixes, we can mark these types of tests as "skip if multiple workers". We can also catch different issues running tests on one worker vs multiple. Narwhals tests are pretty quick to run (~3 minutes to run both single and multiple works)

  • Fixes slicing issue with negative step that returns an empty slice if stop is unbounded and start falls within len(md_head)

Testing strategy

Extended test_slice to add a case for negative step

User facing changes

Bug with slicing a Series with negative step fixed

Checklist

  • Pipelines passed before requesting review. To run CI you must include [run CI] in your commit message.
  • I am familiar with the Contributing Guide
  • I have installed + ran pre-commit hooks.

@codecov
Copy link

codecov bot commented Nov 11, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.81%. Comparing base (c33fbb5) to head (1659cd5).
⚠️ Report is 120 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #923      +/-   ##
==========================================
+ Coverage   66.68%   68.81%   +2.13%     
==========================================
  Files         186      195       +9     
  Lines       66795    67571     +776     
  Branches     9507     9594      +87     
==========================================
+ Hits        44543    46502    +1959     
+ Misses      19572    18241    -1331     
- Partials     2680     2828     +148     

@scott-routledge2 scott-routledge2 marked this pull request as ready for review November 12, 2025 17:32
Copy link
Collaborator

@DrTodd13 DrTodd13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If loc also goes through this same new routine then create a test for it as well?

without negative start/stop indices. stop is None implies the slice
goes from start to the beginning in reverse order.
"""
start, stop, step = slobj.indices(nrows)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will .loc also go through this routine? If so, then is stop = None correct? Should start here also get a similar negative check that stop has?

Copy link
Contributor Author

@scott-routledge2 scott-routledge2 Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the stop is exclusive, there is no way to represent the beginning of a list in reverse order since 0 would exclude the first element and -1 would imply the last element, which is why I am using None here. Since the start is inclusive then I don't think there needs to be a similar negative check.

From testing I think loc takes this same code path but it seems to work. Any particular reason it wouldn't work here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the tests, we cover missing/None indices, positive indices, and negative indices, the part that was missing was negative step, which I add in this PR, so we should have all the cases covered.

Copy link
Collaborator

@ehsantn ehsantn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@scott-routledge2 scott-routledge2 merged commit d9fbd3d into main Nov 13, 2025
30 checks passed
@scott-routledge2 scott-routledge2 deleted the scott/narwhals_single_rank branch November 13, 2025 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants