Repository navigation
Fix/block occ - #387
Merged
Merged
Fix/block occ#387
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
R/PM_final.R fallback upgrade paths overwrite PM_final_data classes and drop engine tags (e.g., NPAG), which breaks downstream plot/summary dispatch.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the Pmetrics R API and data schema to replace the legacy block “observation block” concept with occasion, adding automatic upgrade support for older saved objects/data while keeping block available as a deprecated argument/column for backwards compatibility. It also removes the Rust mimalloc global allocator and makes small build/packaging adjustments.
Changes:
- Introduce
PM_upgrade()and apply it across constructors/plotters to transparently convert legacyblockcolumns tooccasion. - Update public entry points (
make_AUC(),make_NCA(), plot methods,PM_pta$new) to acceptoccasionand deprecateblock, plus add test coverage for deprecation behavior. - Remove the Rust
mimallocallocator dependency and tweak Makevars cleanup behavior.
File summaries
| File | Description |
|---|---|
| tests/testthat/test-occasion-deprecation.R | New tests covering block → occasion deprecation behavior and upgrade semantics. |
| tests/testthat/test-makeAUC.R | Update expectations from block to occasion in AUC outputs. |
| src/rust/src/lib.rs | Remove mimalloc global allocator wiring. |
| src/rust/Cargo.toml | Drop the mimalloc dependency. |
| Cargo.lock | Remove mimalloc/libmimalloc-sys from the lockfile. |
| src/Makevars.in | Adjust cleanup so build artifacts are removed outside the NOT_CRAN conditional. |
| src/Makevars | Same cleanup adjustment as Makevars.in. |
| R/PMutilities.R | Replace block-building helper with makePMdataOccasion(); update getCov() to exclude occasion. |
| R/Pmetrics-package.R | Update imports (.env, DT::datatable, magrittr::not) for new usages. |
| R/PM_upgrade.R | Add PM_upgrade() to upgrade legacy objects/frames from block to occasion. |
| R/PM_sim.R | Switch to imported not() helper when filtering remaining data. |
| R/PM_pta.R | Add occasion parameter; deprecate block; ensure legacy objects are upgraded before filtering. |
| R/PM_post.R | Rename internal block field to occasion; upgrade legacy data; deprecate block plot arg. |
| R/PM_pop.R | Rename internal block field to occasion; upgrade legacy data; deprecate block plot arg. |
| R/PM_op.R | Rename internal block field to occasion; upgrade legacy data; deprecate block plot arg. |
| R/PM_final.R | Upgrade legacy stored final data when reusing objects (but currently strips engine class tags in fallback paths). |
| R/PM_data.R | Make occasion derived metadata; reserve occasion header; ensure writers strip it before engine output. |
| R/PM_cycle.R | Upgrade legacy stored cycle data when reusing objects. |
| R/PM_cov.R | Convert output block column to occasion and upgrade legacy stored cov data when reusing objects. |
| R/make_NCA.R | Add occasion; deprecate block; upgrade legacy input components before filtering. |
| R/make_AUC.R | Add occasion; deprecate block; upgrade legacy data before grouping/filtering. |
| R/globalVariables.R | Register occasion for NSE/CRAN checks. |
| pkgdown/assets/Data/ObjectsR6.csv | Update documented fields from block to occasion. |
| pkgdown/assets/Data/ObjectsLegacy.csv | Update legacy object docs from block to occasion. |
| NAMESPACE | Export PM_upgrade; add imports for DT::datatable, magrittr::not, and rlang::.env. |
| man/PMmatrixRelTime.Rd | Documentation updates describing occasion-based splitting semantics. |
| man/PM_upgrade.Rd | New generated Rd for PM_upgrade(). |
| man/PM_pta.Rd | Document new occasion arg and deprecated block. |
| man/PM_post.Rd | Update field docs from block to occasion. |
| man/PM_pop.Rd | Update field docs from block to occasion. |
| man/PM_op.Rd | Update field docs from block to occasion. |
| man/PM_data.Rd | Add link to online “Pmetrics in R book” format docs. |
| man/plot.PM_post.Rd | Document new occasion arg and deprecated block. |
| man/plot.PM_pop.Rd | Document new occasion arg and deprecated block. |
| man/plot.PM_op.Rd | Document new occasion arg and deprecated block. |
| man/plot.PM_data.Rd | Document new occasion arg and deprecated block. |
| man/make_NCA.Rd | Document new occasion arg and deprecated block. |
| man/make_AUC.Rd | Document new occasion arg and deprecated block. |
| man-roxygen/occasion.R | New roxygen template text for occasion parameter docs. |
| man-roxygen/block.R | Remove old roxygen template text for block. |
| inst/template/Cargo.toml | Add Rust template Cargo manifest. |
| inst/report/templates/ggplot.Rmd | Update report template to count distinct rows using occasion instead of block. |
| .Rbuildignore | Ignore codecov.yml and Quarto vignette working directory. |
Review details
Files not reviewed (4)
- man/PM_data.Rd: Generated file
- man/PM_op.Rd: Generated file
- man/PM_pop.Rd: Generated file
- man/PM_post.Rd: Generated file
Suppressed comments (2)
R/PM_final.R:252
- This fallback path resets the upgraded object’s class to
c("PM_final_data", "list"), which drops the engine tag (e.g.,"NPAG"/"IT2B") needed byplot.PM_final()/summary.PM_final()to choose the correct behavior.PM_upgrade()already preserves attributes/classes on lists, so returning it directly keeps dispatch correct.
upgraded_data <- PM_upgrade(data$data)
class(upgraded_data) <- c("PM_final_data", "list")
return(upgraded_data)
R/PM_final.R:266
- This fallback path resets the upgraded object’s class to
c("PM_final_data", "list"), which drops the engine tag (e.g.,"NPAG"/"IT2B") needed byplot.PM_final()/summary.PM_final()to choose the correct behavior.PM_upgrade()already preserves attributes/classes on lists, so returning it directly keeps dispatch correct.
upgraded_data <- PM_upgrade(data$data)
class(upgraded_data) <- c("PM_final_data", "list")
return(upgraded_data)
- Files reviewed: 38/47 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+236
to
+238
| upgraded_data <- PM_upgrade(data$data) | ||
| class(upgraded_data) <- c("PM_final_data", "list") | ||
| return(upgraded_data) |
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Several new lifecycle::deprecate_warn() calls use a deprecation version (“3.3.0”) that is ahead of the current package version (3.2.6), which can cause lifecycle to error rather than warn.
Review details
Files not reviewed (3)
- man/PM_data.Rd: Generated file
- man/PM_op.Rd: Generated file
- man/PM_pop.Rd: Generated file
Suppressed comments (8)
Previously missed (8) — in code that hasn't changed since the last review.
R/PM_data.R:1718
lifecycle::deprecate_warn()uses version "3.3.0", but the package DESCRIPTION version is 3.2.6; lifecycle expects the deprecation version to be <= the current package version and can error if it’s in the future.
R/PM_op.R:383lifecycle::deprecate_warn()uses version "3.3.0", but the package DESCRIPTION version is 3.2.6; lifecycle expects the deprecation version to be <= the current package version and can error if it’s in the future.
R/PM_pop.R:259lifecycle::deprecate_warn()uses version "3.3.0", but the package DESCRIPTION version is 3.2.6; lifecycle expects the deprecation version to be <= the current package version and can error if it’s in the future.
R/PM_post.R:262lifecycle::deprecate_warn()uses version "3.3.0", but the package DESCRIPTION version is 3.2.6; lifecycle expects the deprecation version to be <= the current package version and can error if it’s in the future.
R/PM_pta.R:174lifecycle::deprecate_warn()uses version "3.3.0", but the package DESCRIPTION version is 3.2.6; lifecycle expects the deprecation version to be <= the current package version and can error if it’s in the future.
R/PM_upgrade.R:104lifecycle::deprecate_warn()uses version "3.3.0", but the package DESCRIPTION version is 3.2.6; lifecycle expects the deprecation version to be <= the current package version and can error if it’s in the future.
R/make_AUC.R:71lifecycle::deprecate_warn()uses version "3.3.0", but the package DESCRIPTION version is 3.2.6; lifecycle expects the deprecation version to be <= the current package version and can error if it’s in the future.
R/make_NCA.R:87lifecycle::deprecate_warn()uses version "3.3.0", but the package DESCRIPTION version is 3.2.6; lifecycle expects the deprecation version to be <= the current package version and can error if it’s in the future.
- Files reviewed: 40/48 changed files
- Comments generated: 0 new
- Review effort level: Lite
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #387 +/- ##
==========================================
+ Coverage 62.73% 63.12% +0.39%
==========================================
Files 51 52 +1
Lines 22410 22545 +135
==========================================
+ Hits 14059 14232 +173
+ Misses 8351 8313 -38
🚀 New features to boost your workflow:
|
mnneely
marked this pull request as ready for review
September 3, 2026 17:04
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new occasion metadata plumbing has a confirmed ordering-sensitivity bug and the new reserved-header check can falsely error on valid case-variant occasion columns.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (2)
- man/PM_data.Rd: Generated file
- man/PM_op.Rd: Generated file
- Files reviewed: 42/49 changed files
- Comments generated: 3
- Review effort level: Lite
Comment on lines
+881
to
+896
| occasion_name <- names(data2)[tolower(names(data2)) == "occasion"] | ||
| if (length(occasion_name) > 0 && !source %in% c("PM_data", "list")) { | ||
| is_occasion_metadata <- identical(occasion_name, "occasion") && | ||
| all(c("id", "evid") %in% names(data2)) && | ||
| isTRUE(all.equal( | ||
| data2$occasion, | ||
| makePMdataOccasion(data2)$occasion, | ||
| check.attributes = FALSE | ||
| )) | ||
| if (!is_occasion_metadata) { | ||
| cli::cli_abort(c( | ||
| "x" = "{.field occasion} is a reserved Pmetrics data header and cannot be used as a covariate name.", | ||
| "i" = "Rename that covariate before calling {.fn PMcheck}." | ||
| )) | ||
| } | ||
| } |
Comment on lines
+323
to
+327
| # Assign observation occasions, delimited by EVID = 4 and stored in `occasion`. | ||
| mdata <- mdata |> | ||
| dplyr::group_by(id) |> | ||
| dplyr::mutate(occasion = 1L + cumsum(dplyr::coalesce(evid == 4, FALSE))) |> | ||
| dplyr::ungroup() |
Comment on lines
2
to
6
| mod logs; | ||
| mod settings; | ||
| mod simulation; | ||
|
|
||
| use mimalloc::MiMalloc; | ||
|
|
||
| /// Use mimalloc as the global allocator for improved allocation performance | ||
| /// across Windows, macOS, and Linux. | ||
| #[global_allocator] | ||
| static GLOBAL: MiMalloc = MiMalloc; | ||
|
|
||
| use anyhow::Result; |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
There are a couple of small but user-visible correctness issues (occasion type stability and lifecycle deprecation version reporting) that should be fixed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (2)
- man/PM_data.Rd: Generated file
- man/PM_op.Rd: Generated file
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
R/PMutilities.R:327
cumsum()returns a double, sooccasionwill be stored as a double even though it is conceptually an integer index. This can cause type mismatches (e.g., strict comparisons or joins) and is easy to avoid by casting to integer after computing the cumulative sum.
- Files reviewed: 42/49 changed files
- Comments generated: 1
- Review effort level: Lite
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
PMcheck() can accept occasion metadata without enforcing it as trailing metadata, which can break covariate indexing assumptions used by downstream validation/reporting.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (2)
- man/PM_data.Rd: Generated file
- man/PM_op.Rd: Generated file
- Files reviewed: 42/49 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
+881
to
+896
| occasion_name <- names(data2)[tolower(names(data2)) == "occasion"] | ||
| if (length(occasion_name) > 0 && !source %in% c("PM_data", "list")) { | ||
| is_occasion_metadata <- identical(occasion_name, "occasion") && | ||
| all(c("id", "evid") %in% names(data2)) && | ||
| isTRUE(all.equal( | ||
| data2$occasion, | ||
| makePMdataOccasion(data2)$occasion, | ||
| check.attributes = FALSE | ||
| )) | ||
| if (!is_occasion_metadata) { | ||
| cli::cli_abort(c( | ||
| "x" = "{.field occasion} is a reserved Pmetrics data header and cannot be used as a covariate name.", | ||
| "i" = "Rename that covariate before calling {.fn PMcheck}." | ||
| )) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
PMcheck() accepts valid derived occasion metadata but does not relocate it, which can cause occasion to be misinterpreted as a covariate (or hide a real covariate) in downstream covariate range logic.
Review details
Suppressed comments (1)
R/PM_data.R:885
- When a valid derived
occasioncolumn is present in a PMmatrix (i.e.,is_occasion_metadatais TRUE),PMcheck()currently leaves it in-place. This can break downstream covariate handling (e.g.,getCov()/errcheck()assume covariates are a contiguous block after the fixed columns), causingoccasionto be treated as a covariate or a real covariate to be skipped unlessoccasionhappens to be the last column. Consider relocatingoccasionto the end once it is validated as metadata.
occasion_name <- names(data2)[tolower(names(data2)) == "occasion"]
if (length(occasion_name) > 0 && !source %in% c("PM_data", "list")) {
is_occasion_metadata <- identical(occasion_name, "occasion") &&
all(c("id", "evid") %in% names(data2)) &&
isTRUE(all.equal(
- Files reviewed: 44/52 changed files
- Comments generated: 0 new
- Review effort level: Lite
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.