Convert numeric interaction timestamps - #1210
Conversation
|
The dev container check failed in tests/tuning/test_optuna_point.py::test_float_points with a Hypothesis FlakyFailure. A generated log scale float fell about 0.0011 below its lower bound, then the failure did not reproduce on Hypothesis retry. This test is outside the timestamp code changed here. I do not have permission to rerun upstream Actions. Could a maintainer rerun the failed job when convenient? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1210 +/- ##
==========================================
- Coverage 90.42% 90.42% -0.01%
==========================================
Files 263 263
Lines 17559 17602 +43
==========================================
+ Hits 15878 15916 +38
- Misses 1681 1686 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
mdekstrand
left a comment
There was a problem hiding this comment.
Thank you for submitting this PR! The dev container test is flaky right now for unclear reasons, so I'm not worried about that one failing if everything else is passing. I've also kicked off the data CI tests (those require manual approval).
The strategy looks sound. I've added comments on some places where we need additional documentation, and one logic change (add_relationships needs to support timestamp inference too).
| @mark.parametrize( | ||
| ("unit", "value"), | ||
| [ | ||
| ("s", 1_700_000_000), |
There was a problem hiding this comment.
There are quite a few magic numbers in the new tests in this file — what is the purpose of selecting these values specifically? Please add code comments describing what these values are chosen to accomplish.
| allow_repeats: bool = True, | ||
| default: bool = False, | ||
| remove_repeats: bool | Literal["exact"] = False, | ||
| timestamp_unit: TimestampUnit | None = None, |
There was a problem hiding this comment.
Let's put Literal[...] directly in the function signatures instead of defining a separate type — that way the acceptable values will be directly in the method documentation.
| integer units are inferred and floating-point values are assumed to | ||
| be seconds. Numeric timestamps are converted to Arrow timestamps. | ||
| """ | ||
| data = _convert_interaction_timestamps(data, timestamp_unit) |
There was a problem hiding this comment.
We need to add the timestamp_unit option to add_relationships as well, and move the conversion logic into that method. It is valid to use add_relationships to add interactions, and the timestamp logic should be identical.
| _TIMESTAMP_MIN_SECONDS = -2_208_988_800 | ||
| _TIMESTAMP_MAX_SECONDS = 4_102_444_800 |
There was a problem hiding this comment.
For clarity, it would be better to define these by creating datetime objects from ISO date strings and extracting the UNIX timestamp, so that it is obvious what these values mean when they are defined.
|
Thanks for the review. I moved timestamp conversion into add_relationships and added timestamp_unit there. add_interactions now forwards the option. I also inlined the accepted values in the public signatures, replaced the numeric bounds with ISO datetimes, and explained the test values. I added direct add_relationships coverage as well. The complete data test suite passed with 440 tests passed, 42 skipped, and 3 expected failures. Ruff checks passed. |
|
The new test jobs are failing before project setup because the mise action is attempting to download version 2026.9.3 and receives a 404. LensKit tests do not start. The local full data suite remains green. |
|
The previous rerun completed with all project and dataset tests passing. Codecov's project check remained red on a 0.01% overall coverage change even though patch coverage passed. I added direct coverage for the remaining timestamp conversion branches: an explicit integer unit, an all-null timestamp column, a nonnumeric timestamp column, and an invalid unit. The updated interaction test file passes all 26 tests, and Ruff passes. |
Fixes #1183.
This converts numeric interaction timestamps to Arrow timestamp types in both
DatasetBuilder.add_interactionsandfrom_interactions_df.timestamp_unitoptionValidation:
Developed with OpenAI Codex assistance. I reviewed and tested the change and remain responsible for it.