Skip to content

Update datetime columns to perserve their original format - #931

Open
sarahmish wants to merge 7 commits into
mainfrom
issue-926-925-datetime-columns
Open

Update datetime columns to perserve their original format#931
sarahmish wants to merge 7 commits into
mainfrom
issue-926-925-datetime-columns

Conversation

@sarahmish

Copy link
Copy Markdown
Contributor

Resolve #925
Resolve #926

@codecov

codecov Bot commented Sep 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.70%. Comparing base (0e6e416) to head (1fc0c8f).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #931      +/-   ##
==========================================
+ Coverage   95.69%   95.70%   +0.01%     
==========================================
  Files         122      122              
  Lines        5316     5332      +16     
==========================================
+ Hits         5087     5103      +16     
  Misses        229      229              
Flag Coverage Δ
integration 81.43% <100.00%> (-0.12%) ⬇️
unit 84.97% <92.59%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@R-Palazzo R-Palazzo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good

I just left minor comments

Comment thread sdmetrics/demos.py Outdated


@pytest.fixture
def converted_datetime_single_table_demo():

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we name it data_single_table_demo instead? Since it’s used in many places that don’t specifically focus on datetime conversion.

@sarahmish sarahmish Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I can do that but I kept it verbose to make it clearer.

For example, boundary, column shapes, coverage, etc tests fail if the datetime column is not in a datetime64 type. This makes it easy to follow what's the data that's being used in the test.

Let me know if you still prefer this change

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Instead of converted could we be explicit and say object_datetime or string_datetime?

1.0,
np.nan,
1.0,
np.nan,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That is the change from #925?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yep! DatetimeFormatAdherence should be NaN if the column is in datetime64 format

@sarahmish
sarahmish force-pushed the issue-924-unknown-denominator branch from bf4006d to 07e8d51 Compare September 4, 2026 17:27
Base automatically changed from issue-924-unknown-denominator to main September 4, 2026 17:44
@sarahmish
sarahmish force-pushed the issue-926-925-datetime-columns branch from 950df77 to 6e2bdd2 Compare September 8, 2026 17:40
@sarahmish
sarahmish requested a review from R-Palazzo September 8, 2026 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants