Skip to content

min max date - #1825

Open
SFJohnson24 wants to merge 5 commits into
mainfrom
min
Open

min max date#1825
SFJohnson24 wants to merge 5 commits into
mainfrom
min

Conversation

@SFJohnson24

@SFJohnson24 SFJohnson24 commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

this PR works to fix the issues encountered in open rules with CORE-000238 and CORE-000370 (note 238 has bad data for negative 02 which I resolved in another PR as well as negative 1 having incorrect results). CORE-000178 in deprecated also checks these operators.

this PR does a few things:

  • Fixes min_date/max_date grouped aggregation — previously .groupby(grouping).min()/.max() was called directly on the raw dataframe, which aggregated across all columns instead of just the target column, and would crash on mixed-type groups (e.g., blank cells parsed as float('nan') alongside string dates). The new logic now converts the target column to datetime first, groups only that column, and uses .transform() instead of just .min()/.max() so the per-subject result broadcasts back to every row with the correct index alignment (previously produced all NaT).

  • Fixed date parsing for mixed-precision and malformed values — added format="ISO8601" to correctly parse date/time values with varying precision

  • Fixes output formatting to preserve date precision — added format_date utility used by both date operations, which only appends a time component when one actually exists. Timestamp.isoformat() always added T00:00:00 even for date-only values, causing detect_datetime_precision mismatches that made date_not_equal_to/equal_to comparisons return incorrect results regardless of the actual dates.

  • adds logic to match on grouping--so when the max_date is grouped on USUBJID for a target dataset, it will then attach the value to corresponding USUBJID in the dataframe the operation is being added to.

fixed stale tests, added a grouping case to the min_date test

@SFJohnson24

SFJohnson24 commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator Author

I ran the updated rule against the rules referenced above CORE-000238/CORE-000370 on my branch with the fixed data and everything passes. https://github.com/cdisc-org/cdisc-rules-engine/actions/runs/31725239002

CORE-000178 is in the test suite and passes

NOTE: CORE-000363 also failing and this PR also fixes that

@pendingintent

Copy link
Copy Markdown
Collaborator

My initial review and test were fine but my assitant found:

group_aliases silently dropped for min_date/max_date

Grouped operations in the engine (minimum.py, maximum.py, record_count.py) return a DataFrame, which routes through BaseOperation._handle_grouped_result

Here the min_date/max_date now return a pd.Series directly, which trips the is_series branch in _handle_operation_result first, bypassing _handle_grouped_result entirely.

Any rule config that sets group_aliases on a min_date/max_date operation would have that alias silently ignored.

In cdisc-open-rules, no published rule combines min_date/max_date with group_aliases so nothing breaks.

@pendingintent

Copy link
Copy Markdown
Collaborator

Minute/second-precision gap.

:00 seconds get appended, which detect_datetime_precision will read as second-precision instead of minute-precision.

SDTM --DTC support minute-precision but it is not correct to infer time. When submitting SDTM datasets, all values are as collected or marked as derived. There may be edge cases where appending an 'incorrect' value of :00 leads to submission issues.

@pendingintent pendingintent left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

My initial review and test were fine but my assitant found:

group_aliases silently dropped for min_date/max_date

Grouped operations in the engine (minimum.py, maximum.py, record_count.py) return a DataFrame, which routes through BaseOperation._handle_grouped_result

Here the min_date/max_date now return a pd.Series directly, which trips the is_series branch in _handle_operation_result first, bypassing _handle_grouped_result entirely.

Any rule config that sets group_aliases on a min_date/max_date operation would have that alias silently ignored.

In cdisc-open-rules, no published rule combines min_date/max_date with group_aliases so nothing breaks.

Minute/second-precision gap.

:00 seconds get appended, which detect_datetime_precision will read as second-precision instead of minute-precision.

SDTM --DTC support minute-precision but it is not correct to infer time. When submitting SDTM datasets, all values are as collected or marked as derived. There may be edge cases where appending an 'incorrect' value of :00 leads to submission issues.

@SFJohnson24

Copy link
Copy Markdown
Collaborator Author

group_aliases is not a valid argument for min/max date

The precision is a concern that I overlooked. I funneled the operations into the check_operator helpers as there is a fair bit of date_time precision logic there. I created a new function that does datetime, preserving the precision of the string that is the min/max and returns it to the operation.

@pendingintent pendingintent left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It is possible that no dates are present for a USUBJID. I believe this is a condition that should be guarded against.

grouping_cols = [grouping_cols]

group_keys = [self.params.dataframe[col] for col in grouping_cols]
idx_of_max = data.groupby(group_keys).idxmax()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

the grouped branch has no guard for a group whose dates are entirely blank

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same issue as the min_date.py thread — the pd.isna(idx) guard here is unreachable because .groupby(group_keys).idxmax() raises ValueError: idxmax with skipna=True encountered all NA values in a group. for an all-blank group under pandas>=3.0.0, before ever returning a value for .apply() to check. A USUBJID with no non-blank dates in the grouped case will crash rather than resolve to "".

grouping_cols = [grouping_cols]

group_keys = [self.params.dataframe[col] for col in grouping_cols]
idx_of_min = data.groupby(group_keys).idxmin()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

the grouped branch has no guard for a group whose dates are entirely blank

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Confirmed this is still open. The pd.isna(idx) check in the .apply() here never gets reached — .groupby(group_keys).idxmin() itself raises ValueError: idxmin with skipna=True encountered all NA values in a group. when a group is entirely blank/NaT, under pandas>=3.0.0 (per pyproject.toml). Verified locally:

>>> data.groupby(group_keys).idxmax()
ValueError: idxmax with skipna=True encountered all NA values in a group.

So a USUBJID with zero non-blank dates in the grouped case will crash the whole operation rather than resolve to "". Suggest filtering out all-NaT groups (e.g. drop NA from data before grouping, or compute per-group validity and only call idxmin/idxmax on groups with at least one non-NaT value) before this line.

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.

2 participants