Skip to content

Bug fix: prevent PKNCA_impute_method_FALSE error from YAML impute_c0 uploaded issue #1266#1322

Open
wangzhengdna-lang wants to merge 12 commits into
pharmaverse:mainfrom
wangzhengdna-lang:1266-fix/blq-impute-false-column-guard
Open

Bug fix: prevent PKNCA_impute_method_FALSE error from YAML impute_c0 uploaded issue #1266#1322
wangzhengdna-lang wants to merge 12 commits into
pharmaverse:mainfrom
wangzhengdna-lang:1266-fix/blq-impute-false-column-guard

Conversation

@wangzhengdna-lang

Copy link
Copy Markdown

Issue

Closes #1266

Description

Uploading a settings YAML with impute_c0: no (e.g., All_settings_changed.yaml attached to the issue) causes NCA to fail with:

PKNCA_impute_fun_list(): The following imputation functions were not found: PKNCA_impute_method_FALSE

Root cause

impute_c0: no in YAML parses to R logical FALSE. This flows to update_main_intervals(impute = FALSE), which skips create_start_impute(). Since create_start_impute() is responsible for adding the impute column to data$intervals, the column never exists.

When BLQ imputation is subsequently applied via dplyr::mutate(impute = ifelse(is.na(impute) | impute == "", "blq", paste0("blq, ", impute))), dplyr cannot find an impute column in the data frame and falls back to the function parameter impute = FALSE from the calling environment. This produces paste0("blq, ", FALSE)"blq, FALSE". PKNCA's PKNCA_impute_fun_list() splits this by comma, finds "FALSE", and attempts to locate the non-existent function PKNCA_impute_method_FALSE.

Fix

One-line guard in update_main_intervals() (intervals.R:292-298) — initialize the impute column before the BLQ code block, identical to the existing guard already present in rm_impute_obs_params() (same file, line 341):

if (!"impute" %in% names(data$intervals)) {
    data$intervals$impute <- NA_character_
}

Definition of Done

  • Settings YAML with impute_c0: no no longer causes PKNCA_impute_method_FALSE error
  • Settings YAML with impute_c0: yes still works correctly (no regression)
  • BLQ imputation with impute_c0 = FALSE works correctly (impute column gets "blq" only)
  • All unit tests pass (0 failures)
  • Fix follows existing guard pattern in rm_impute_obs_params()

How to test

  1. Load: devtools::load_all()
  2. Reproduce the fix: Launch the app, upload the All_settings_changed.yaml from the issue Bug: error in `PKNCA_impute_fun_list() #1266 , run NCA — should complete without the PKNCA_impute_method_FALSE error
  3. Regression test: Toggle "Impute Start Concentration" on/off in Data Imputation settings, run NCA — both paths should work normally

Contributor checklist

  • Code passes lintr checks
  • Code passes all unit tests — 0 failures, 68 warnings (all pre-existing), 3 skips
  • New logic covered by unit tests — N/A (defensive guard only, no new logic)
  • App or package changes are reflected in NEWS
  • Package version is incremented (0.1.0.9173 → 0.1.0.9174)
  • R script works with the new implementation — N/A (no NCA logic changes)
  • Settings upload works with the new implementation — this is the fix path
  • If any .scss change was done, run data-raw/compile_css.RN/A
  • If a package dependency was added/changed, run data-raw/test_suggests_hidden.RN/A

Notes to reviewer

  • The fix is a single defensive guard (if (!"impute" %in% names(...))) that mirrors an identical pattern in rm_impute_obs_params() (same file, 20 lines below). No new patterns introduced.
  • The deeper cause is a name collision between the impute function parameter and a data frame column. A follow-up could rename the parameter to start_impute to avoid this class of bug entirely, but that would touch more call sites and is out of scope for this minimal fix.
  • The issue#1266/All_settings_changed.yaml file attached to the issue was used to confirm the root cause but is not included in this commit.

王崝 and others added 2 commits May 26, 2026 15:43
pharmaverse#1266)

When start_impute=FALSE, create_start_impute() is skipped and
data$intervals has no impute column. dplyr mutate falls back to the
function parameter impute=FALSE, producing "blq, FALSE" which PKNCA
interprets as an imputation method name lookup for the non-existent
PKNCA_impute_method_FALSE.

Fix: initialize the impute column before BLQ imputation is applied,
matching the existing guard pattern in rm_impute_obs_params().

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…CLAUDE.md

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@wangzhengdna-lang wangzhengdna-lang marked this pull request as ready for review May 26, 2026 08:50

@Shaakon35 Shaakon35 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.

Thank you for the fix,

I still have an issue with this settings:

ERROR [2026-05-27 10:28:52] Error calculating NCA results:
Can't join x$AGE with y$AGE due to incompatible types.
x$AGE is a .
y$AGE is a .

also, somehow the nca is not re-run automatically (maybe because of the bug of the AGE), let's see when that is fixed.

Comment thread CLAUDE.md Outdated
@Shaakon35 Shaakon35 requested a review from Gero1999 May 27, 2026 08:40
王崝 and others added 3 commits May 27, 2026 17:09
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…rmaverse#1266)

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…e#1266)

When settings YAML contains group columns like AGE in RefGroups
(e.g. "AGE: 25"), the parsed values are character strings. Joining
these with PKNCA result data (where AGE is numeric) triggers dplyr's
strict type check: "Can't join due to incompatible types".

Two fixes:
1. ratio_calculations.R: Add .coerce_group_types() to convert YAML
   character values to match the corresponding column types in data
   before merge() and anti_join() operations.
2. PKNCA.R: Restrict remove_pp_not_requested() group_by to only
   PKNCA-essential columns, excluding user keep_interval_cols (like
   AGE) that are irrelevant for parameter-request matching.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@wangzhengdna-lang

wangzhengdna-lang commented May 27, 2026

Copy link
Copy Markdown
Author

Thanks for your review. Upon further analysis, the issues below were confirmed and resolved,

About the AGE incompatible types error:

The root cause was in the ratio calculation pipeline. When the settings YAML specifies group columns like RefGroups: "AGE: 25", the value "25" is parsed as a character string (.parse_ratio_groups() creates data.frame(AGE = "25") using matrix() which produces all-character columns). However, the corresponding AGE column in the PKNCA result data is <double>. When calculate_ratios.data.frame() joins df_test with ref_groups via anti_join() (line 142 of ratio_calculations.R), dplyr's strict type checking detects the <double> vs <character> mismatch and throws "Can't join due to incompatible types".

The fix adds a .coerce_group_types() helper that converts the character values from YAML-parsed group data frames to match the actual column types in the PKNCA result data (e.g., as.numeric() for numeric columns like AGE). This is called before both merge() and anti_join() operations.

Additionally, remove_pp_not_requested() in PKNCA.R was updated to use only PKNCA-essential columns (formula groups + interval identifiers) in its group_by() instead of across(c(-impute, -is_requested)) which previously included all user-specified keep_interval_cols (like AGE, SEX) in the join keys — a second code path that could trigger the same type of error.

About NCA not re-running automatically:

This was a consequence of the AGE error — the join failure aborted the auto-replay flow before NCA could complete. With the type coercion fix, auto-replay should now work correctly. I've verified locally that uploading the same dataset + settings YAML now succeeds with SUCCESS: NCA results calculated.

Note: In my testing, the settings YAML specifies RefGroups: "AGE: 25" and TestGroups: "(all other levels)", so if the uploaded dataset has no subjects with AGE=25, a warning Ratio RAAUCINT_0-12 not computed. No comparable groups found will appear — this is expected behavior when the ratio reference group doesn't exist in the data, not a bug.

@Shaakon35

Copy link
Copy Markdown
Collaborator

Thanks, now no crash but there is an issue with the AUCINT, it's complelty buggy, let me investigate :/

Shaakon35 and others added 2 commits June 3, 2026 10:38
- slope_selector.R: use reset_reactable_memory() instead of inline JS
- settings.R: add missing reset_reactable_memory() in .restore_non_filter_settings
  to prevent stale reactable.extras widget values on settings upload

Co-authored-by: Ona <no-reply@ona.com>
@Shaakon35

Copy link
Copy Markdown
Collaborator

The original crash from #1266 (PKNCA_impute_method_FALSE error) is fixed by this PR. However, testing with the same All_settings_changed.yaml from #1266 revealed two additional issues:

  1. Spurious AUCINT intervals — The settings YAML contains int_parameters with .na.real start/end. After upload, the reactable.extras widgets appear to replace the NA values with stale numeric values, causing unwanted AUCINT_x-y intervals to be generated and computed. Tracked in Bug: Spurious AUCINT intervals generated when uploading settings with empty start/end #1347.

  2. Auto-restore does not run NCA — After uploading settings, the NCA is not triggered automatically. The user must manually click "Run NCA". Likely a reactive timing issue. Tracked in Bug: Auto-restore does not run NCA after uploading settings #1348.

Both are pre-existing issues not introduced by this PR.

@Shaakon35 Shaakon35 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.

Thanks for fixing this, very helpful!

Shaakon35 and others added 4 commits June 3, 2026 09:38
Co-authored-by: Ona <no-reply@ona.com>
Co-authored-by: Ona <no-reply@ona.com>
Co-authored-by: Ona <no-reply@ona.com>
Co-authored-by: Ona <no-reply@ona.com>
@Shaakon35

Copy link
Copy Markdown
Collaborator

⚠️ Note for future PRs: CLAUDE.md was accidentally added to .gitignore in this branch. It has been removed, but please double-check that local editor config files (e.g. CLAUDE.md) don't end up in .gitignore commits going forward.

@Shaakon35

Copy link
Copy Markdown
Collaborator

The intervals.R guard and the PKNCA.R grouping refactor look good — minimal and consistent with existing patterns.

One request: .coerce_group_types() in ratio_calculations.R has branching logic across multiple type conversions (character → numeric, character → factor with levels, already-matching types, etc.). Could you add unit tests for this helper? A few cases would be enough:

  • Character column matching a numeric column in data → coerced to numeric
  • Character column matching a factor column → coerced to factor with correct levels
  • Already-matching types → no change
  • Mixed: some columns need coercion, some don't

The intervals.R guard is genuinely just a defensive one-liner so no tests needed there, but .coerce_group_types() is ~15 lines of new logic that handles edge cases worth covering.

@wangzhengdna-lang wangzhengdna-lang left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@Shaakon35 Added 4 unit tests for .coerce_group_types() covering all requested cases:

  • Character → numeric (AGE "25" coerced to double)
  • Character → factor with correct levels (RACE "WHITE" matched to factor levels)
  • Already-matching types unchanged (numeric stays numeric, character stays character)
  • Mixed columns (AGE coerced, RACE/SEX unchanged)

All 151 ratio tests pass.

@Shaakon35

Copy link
Copy Markdown
Collaborator

@h5hoang Could you please have a review? :)

@Shaakon35 Shaakon35 requested a review from h5hoang July 1, 2026 11:38
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.

Bug: error in `PKNCA_impute_fun_list()

3 participants