feat: implement knit_print methods for Simulations classes#931
Open
pengguanya wants to merge 14 commits intomainfrom
Open
feat: implement knit_print methods for Simulations classes#931pengguanya wants to merge 14 commits intomainfrom
pengguanya wants to merge 14 commits intomainfrom
Conversation
Implements knit_print S3 methods for all Simulations and SimulationsSummary subclasses to enable human-friendly rendering in Markdown and Quarto documents. Classes implemented: - GeneralSimulations - Simulations - DualSimulations - PseudoSimulations - PseudoDualSimulations - PseudoDualFlexiSimulations - DASimulations - GeneralSimulationsSummary - SimulationsSummary - DualSimulationsSummary - PseudoSimulationsSummary - PseudoDualSimulationsSummary Closes #930
Contributor
Unit Test Performance Difference
Additional test case details
Results for commit b3621a6 ♻️ This comment has been updated with latest results. |
Contributor
|
Code Coverage SummaryDiff against mainResults for commit: 8917857 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
Contributor
Unit Tests Summary 1 files 55 suites 4m 31s ⏱️ For more details on these errors, see this check. Results for commit e62ee9d. ♻️ This comment has been updated with latest results. |
Collaborator
danielinteractive
left a comment
danielinteractive
left a comment
There was a problem hiding this comment.
Thanks @pengguanya , please see couple of comments below.
Please can you also check that
- the representation in
rmd/qmdfiles looks "good" - the help page
knit_printlooks "good"
| # DualSimulationsSummary ---- | ||
|
|
||
| test_that("knit_print.DualSimulationsSummary works correctly", { | ||
| skip("DualSimulations summary requires complex setup with biomarker truth") |
Collaborator
There was a problem hiding this comment.
Here and below we also need real tests. If it takes too long to run simulations here you can follow the pattern used for the other mock simulation objects, see https://github.com/openpharma/crmPack/blob/main/tests/testthat/test-CrmPackClass-class.R
Collaborator
Author
There was a problem hiding this comment.
@danielinteractive Updated. Please review again.
|
|
||
| result <- knit_print(x, asis = FALSE) | ||
|
|
||
| expect_true(grepl("### Simulation Results", result, fixed = TRUE)) |
Collaborator
There was a problem hiding this comment.
Instead of checking for these strings I would prefer snapshot tests such that I can see how the results look like (see as reference https://github.com/openpharma/crmPack/tree/main/tests/testthat/_snaps/Darwin/helpers_knitr where we have a lot of other snapshots already saved)
Collaborator
Author
There was a problem hiding this comment.
This is not done yet. will take more time.
Collaborator
|
Hi @pengguanya , I merged main into your branch, so you could continue from there |
Convert string-based grepl() assertions to expect_snap() for deterministic simulation objects (GeneralSimulations, DualSimulations, PseudoSimulations, PseudoDualSimulations, PseudoDualFlexiSimulations). Keep grepl() assertions for MCMC-dependent tests (Simulations, DASimulations, Summary classes) to avoid brittle tests from stochastic variation. Add platform-specific snapshot files for Linux, Darwin, and Windows.
Co-authored-by: Daniel Sabanes Bove <danielinteractive@users.noreply.github.com>
Replace 6 skipped tests with real tests using pre-computed RDS fixtures: - DualSimulationsSummary - PseudoSimulationsSummary - PseudoDualSimulationsSummary Also fix .DefaultDualSimulationsSummary() to throw an error for consistency with other summary class constructors.
Convert grepl-based assertions to expect_snap() for better visibility of knit_print output as requested in code review.
- Convert grepl-based tests to expect_snap() for consistency: - knit_print.Simulations - knit_print.DASimulations - knit_print.GeneralSimulationsSummary - knit_print.SimulationsSummary - Add mocks for .DefaultSimulations, .DefaultDASimulations, and .DefaultSimulationsSummary to ensure stable snapshots - Add SimulationsSummary fixture generation - Regenerate all fixtures for consistency
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
2 participants
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.
Pull Request
Implements knit_print S3 methods for all Simulations and SimulationsSummary subclasses to enable human-friendly rendering in Markdown and Quarto documents.
Classes implemented:
Fixes #930