Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add datetime selector #1822

Merged
merged 31 commits into from
Feb 9, 2025
Merged

feat: add datetime selector #1822

merged 31 commits into from
Feb 9, 2025

Conversation

FBruzzesi
Copy link
Member

@FBruzzesi FBruzzesi commented Jan 17, 2025

What type of PR is this? (check all applicable)

  • 💾 Refactor
  • ✨ Feature
  • 🐛 Bug Fix
  • 🔧 Optimization
  • 📝 Documentation
  • ✅ Test
  • 🐳 Other

Related issues

Checklist

  • Code follows style guide (ruff)
  • Tests added
  • Documented the changes

If you have comments or can explain your changes, please do so below

@MarcoGorelli you might have a better idea for how to test this. I checked how polars does it and it is definitly brute forcing 😂

I took the opportunity to:

  • Modernize the selectors examples in the docstrings
  • Create a TimeUnit alias to use around the codebase

The actual changes are not too large

@FBruzzesi FBruzzesi added the enhancement New feature or request label Jan 17, 2025
narwhals/utils.py Outdated Show resolved Hide resolved
self: Self,
time_unit: TimeUnit | Collection[TimeUnit] | None,
time_zone: str | timezone | Collection[str | timezone | None] | None,
) -> DaskSelector: # pragma: no cover
Copy link
Member Author

@FBruzzesi FBruzzesi Jan 17, 2025

Choose a reason for hiding this comment

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

For dask, the selector works, but the cast fails with:

TypeError: Cannot use .astype to convert from timezone-aware dtype to timezone-naive dtype. Use obj.tz_localize(None) or obj.tz_convert('UTC').tz_localize(None) instead.

@FBruzzesi FBruzzesi marked this pull request as ready for review January 19, 2025 11:01
pyproject.toml Outdated Show resolved Hide resolved
narwhals/utils.py Outdated Show resolved Hide resolved
Comment on lines +1106 to +1111
(dtype == dtypes.Datetime)
and (dtype.time_unit in time_units) # type: ignore[attr-defined]
and (
dtype.time_zone in time_zones # type: ignore[attr-defined]
or ("*" in time_zones and dtype.time_zone is not None) # type: ignore[attr-defined]
)
Copy link
Member Author

Choose a reason for hiding this comment

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

@MarcoGorelli this might make the trick you were looking for :)

Copy link
Member

Choose a reason for hiding this comment

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

yup, nice!

Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

nice, thanks @FBruzzesi ! sorry didn't quite get round to reviewing everything in time for today, we can always include in next week's release (in fact, that's one reason to have regular and short release cycles, so we don't feel like we need to rush things in - if something doesn't make it, the next release is just next week)

tests/selectors_test.py Show resolved Hide resolved
Comment on lines +1106 to +1111
(dtype == dtypes.Datetime)
and (dtype.time_unit in time_units) # type: ignore[attr-defined]
and (
dtype.time_zone in time_zones # type: ignore[attr-defined]
or ("*" in time_zones and dtype.time_zone is not None) # type: ignore[attr-defined]
)
Copy link
Member

Choose a reason for hiding this comment

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

yup, nice!

{None}
if time_zone is None
else {str(time_zone)}
if isinstance(time_zone, (str, timezone))
Copy link
Member

Choose a reason for hiding this comment

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

do we have any test for when it's an instance of timezone? can we check that timezone.utc and ZoneInfo.UTC both work?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well.. ZoneInfo does not work for polars 😱

Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

nice one, implementation looks good!

couple of comments:

  • as this is new functionality, shall we make the docstring concise (like the ones in narwhals/expr.py) to begin with?
  • got a comment about time_unit

narwhals/utils.py Outdated Show resolved Hide resolved
Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

thanks!

narwhals/selectors.py Show resolved Hide resolved
tests/selectors_test.py Outdated Show resolved Hide resolved
narwhals/selectors.py Outdated Show resolved Hide resolved
Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

thanks @FBruzzesi

@MarcoGorelli
Copy link
Member

sorry, just some more merge conflicts

@FBruzzesi FBruzzesi changed the title feat: datetime selector feat: add datetime selector Feb 9, 2025
@FBruzzesi FBruzzesi merged commit df8144b into main Feb 9, 2025
28 checks passed
@FBruzzesi FBruzzesi deleted the feat/selectors-by-datetime branch February 9, 2025 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Enh]: ncs.datetime selector
2 participants