Handle pandas timestamp with nanosecs precision #49370
Conversation
Signed-off-by: Srinath Krishnamachari <[email protected]>
There was a problem hiding this comment.
Few comments about the tests, I also wonder if we could somehow fold all these tests into a single parameterized test that is agnostic of the type (e.g. use the expected type in the parameters somewhere to know what type to expect. Would help with the standardization across the tests.
| # Convert Python datetime to pandas Timestamp with nanosecond precision | ||
| value = pd.Timestamp(value) | ||
| value = pa.array([value], type=pa.timestamp("ns"))[0] | ||
|
|
There was a problem hiding this comment.
Are there any ways that this conversion can fail or precision can be lost? I imagine probably not but might be good to note anything here if there are.
There was a problem hiding this comment.
Hey again, piping in from the original issue.
If this is already a datetime object, hasn't it already lost nanosecond precision? Is it too late to perform this coercion?
There was a problem hiding this comment.
Default behavior is type coercion as we discovered in the bug from the test case. This is explicitly handling the type conversion for nanoseconds.
python/ray/data/tests/test_map.py
Outdated
| "df, expected_df", | ||
| [ | ||
| ( | ||
| create_timestamp_dataframe(), |
There was a problem hiding this comment.
Unless I am missing something this looks identical to the result of create_timestamp_dataframe? Should we just call that function once? Should we only pass in one dataframe?
There was a problem hiding this comment.
Ah I see now that there is a 1ns difference, can we include a comment to specify that? Or could just have one DF passed in and do the addition of 1ns in the test itself?
There was a problem hiding this comment.
I will check if I can simplify this without limiting the correctness check.
python/ray/data/tests/test_map.py
Outdated
| processed_df = result.to_pandas() | ||
| assert processed_df.shape == df.shape, "DataFrame shapes do not match" | ||
| pd.testing.assert_frame_equal(processed_df, expected_df) | ||
| assert (processed_df["timestamp"] - df["timestamp"]).iloc[0] == pd.Timedelta( |
There was a problem hiding this comment.
Are these last three asserts redundant with the above equality check?
There was a problem hiding this comment.
Ah this is checking for all 3 rows.
python/ray/data/tests/test_map.py
Outdated
| result = ray_data.map(process_timestamp_data) | ||
| processed_df = result.to_pandas() | ||
| # Ensure numpy.datetime64 is correctly converted to pandas Timestamp | ||
| assert isinstance(processed_df["timestamp"].iloc[0], pd.Timestamp) |
There was a problem hiding this comment.
Should we just check if the series for the column has the correct type?
python/ray/data/tests/test_map.py
Outdated
| assert isinstance(processed_df["timestamp"].iloc[0], pd.Timestamp) | ||
|
|
||
| # Check that the timestamp has been incremented by 1ns | ||
| assert (processed_df["timestamp"] - df["timestamp"]).min() == pd.Timedelta(1, "ns") |
There was a problem hiding this comment.
Can we standardize how we are checking the diff of 1ns? Seems like we are doing this, the equality check with the expected check and also individually checking the differences. Not super opinionated on which is the clearest but let's just use one (unless I am missing some subtlety here)
.
There was a problem hiding this comment.
This one is datetime vs np.datetime64. Let me consolidate this code better.
Signed-off-by: Srinath Krishnamachari <[email protected]>
Signed-off-by: srinathk10 <[email protected]>
Signed-off-by: srinathk10 <[email protected]>
Signed-off-by: Srinath Krishnamachari <[email protected]>
Signed-off-by: srinathk10 <[email protected]>
Signed-off-by: Srinath Krishnamachari <[email protected]>
Signed-off-by: Srinath Krishnamachari <[email protected]>
Signed-off-by: Srinath Krishnamachari <[email protected]>
Signed-off-by: Srinath Krishnamachari <[email protected]>
Signed-off-by: Srinath Krishnamachari <[email protected]>
| ) | ||
| ], | ||
| ) | ||
| def test_map_numpy_datetime(df, expected_df, ray_start_regular_shared): |
There was a problem hiding this comment.
Nit: It seems like test_map_numpy_datetime and test_map_timestamp_nanosecs have the same body? Can probably move this to be one test with two different sets of parameters.
| if isinstance(value, (pd.Timestamp, np.datetime64)): | ||
| # If it's a pandas Timestamp or numpy datetime64, convert to pyarrow | ||
| # Timestamp | ||
| value = pa.array([value], type=pa.timestamp("ns"))[0] |
There was a problem hiding this comment.
if the target block type is pandas, will converting it to pyarrow Timestamp be compatible?
There was a problem hiding this comment.
We should probably put this in the arrow_block.py subclass
There was a problem hiding this comment.
also, can you comment that the purpose of this conversion is to avoid losing precision?
| ) | ||
| ], | ||
| ) | ||
| def test_map_timestamp_nanosecs(df, expected_df, ray_start_regular_shared): |
There was a problem hiding this comment.
- can you document the purpose of each test?
- maybe put them in
test_pandas.py, as the issue is specific to Pandas.
Why are these changes needed?
Handle pandas timestamp with nanosecs precision
Related issue number
"Closes #49297"
Checks
git commit -s) in this PR.scripts/format.shto lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/under thecorresponding
.rstfile.