min max date - #1825
Conversation
|
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 |
|
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. |
pendingintent
left a comment
There was a problem hiding this comment.
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.
|
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
left a comment
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
the grouped branch has no guard for a group whose dates are entirely blank
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
the grouped branch has no guard for a group whose dates are entirely blank
There was a problem hiding this comment.
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.
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