Skip to content

Multiple small improvement#115

Merged
PavelBal merged 19 commits into
mainfrom
add-default-outputs
Feb 24, 2026
Merged

Multiple small improvement#115
PavelBal merged 19 commits into
mainfrom
add-default-outputs

Conversation

@dlefaudeux

Copy link
Copy Markdown
Collaborator

@dlefaudeux dlefaudeux linked an issue Feb 17, 2026 that may be closed by this pull request
@dlefaudeux
dlefaudeux marked this pull request as ready for review February 19, 2026 09:30
@nautilus69

Copy link
Copy Markdown
Collaborator

1- #' @param individual_name name(s) of the formulation(s) to remove
Should say "individual(s)", not "formulation(s)".

2- Missing NULL fallback in resolution switch() (R/outputSchema.R:34-54)

Both switch() calls in the data active binding have no default case. If x$unit is a valid time unit not covered
(e.g., "ms" — milliseconds is a valid ospsuite time unit), the switch() silently returns NULL, which would produce
malformed JSON output.

3- Resolution conversion logic may be incorrect for week(s) (R/outputSchema.R:40) : The hardcoded parts require revisions

  1. No validation on individual parameter in add_individual (R/snapshot.R:155) (I am not sure about old behavior)

5- tests for add-individual / remove_individual and for resolution unit conversion logic (for week and year)

The add_formulation method (via add_item) presumably validates the object type. But with the new wrapping of
individuals via Individual$new(), there's no check that the individual argument to add_individual() is actually an
Individual object. If a user passes raw list data (which was the old format), it would silently add invalid data.

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

1 & 2 + tests needed to be addressed,

the rest are optional for now.

@dlefaudeux

dlefaudeux commented Feb 23, 2026

Copy link
Copy Markdown
Collaborator Author

2- Missing NULL fallback in resolution switch() (R/outputSchema.R:34-54)

Only schema with units in ospsuite::getUnitsForDimension("Time") can be added, switch list all possible units. Do I really need a fallback I feel like it should never be triggered

ms is not in ospsuite::getUnitsForDimension("Time")

@dlefaudeux

Copy link
Copy Markdown
Collaborator Author

3- Resolution conversion logic may be incorrect for week(s) (R/outputSchema.R:40) : The hardcoded parts require revisions

Why ?
I can use that below but I felt like it is putting overheads for nothing
x$resolution / ospsuite::toUnit("Time", 1, sourceUnit = "week(s)", targetUnit = "day(s)") instead of x$resolution / 7

same reason for "ks"

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR bundles several small enhancements across the snapshot/simulation workflow: fixing output schema resolution unit handling, expanding default outputs (plasma + plasma unbound) for victim/perpetrator, adding a default “victim only” simulation template, improving formulation docs, and integrating osp.snapshots Individuals/Populations into Snapshot.

Changes:

  • Adjust output schema resolution serialization to normalize certain time units (e.g., week/month/year/ks) and extend related tests/snapshots.
  • Update generic simulation template + simulation initialization defaults to include Plasma Unbound outputs, and add a default “victim only” simulation.
  • Wrap snapshot Individuals/Populations using osp.snapshots classes and add Snapshot$add_individual() / Snapshot$remove_individual() with documentation updates.

Reviewed changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
tests/testthat/test-snapshot.R Adds a test around individual add/remove behavior.
tests/testthat/test-outputSchema.R Updates interval snapshot expectations and adds resolution conversion tests.
tests/testthat/_snaps/simulation_template.md Updates snapshot output for new template defaults (unbound + victim-only sim).
tests/testthat/_snaps/simulation.md Updates printed simulation outputs to include Plasma Unbound selections.
tests/testthat/_snaps/run-simulations.md Updates result snapshots reflecting new default outputs and messages.
tests/testthat/_snaps/outputSchema.md Updates printed output schema snapshot for new interval example.
man/create_formulation.Rd Restructures help content into clearer sections per formulation type.
man/add_observed_data.Rd Fixes value description text for observed data helper.
man/Snapshot.Rd Documents new add_individual / remove_individual methods.
man/DDI.Rd Adds inherited method links for individual add/remove.
inst/extdata/generic_simulation_template.json Adds Plasma Unbound outputs and a default “victim only” simulation.
R/snapshot.R Converts Individuals/Populations to osp.snapshots R6 objects; adds add/remove individual methods; updates export.
R/simulations.R Initializes simulations with default Plasma + Plasma Unbound output selections.
R/outputSchema.R Converts resolution value/unit for specific time units when exporting schema JSON.
R/formulation.R Improves roxygen help structure for create_formulation().
R/ddi.R Switches individual name lookup to the new individual representation ($name).
R/cts-package.R Adds new osp.snapshots imports.
NAMESPACE Adds osp.snapshots importFrom entries.
DESCRIPTION Adds osp.snapshots to Imports and lists it under Remotes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread R/outputSchema.R
Comment thread tests/testthat/test-snapshot.R
Comment thread tests/testthat/test-snapshot.R Outdated
Comment thread R/cts-package.R
Comment on lines +9 to +10
#' @importFrom osp.snapshots AdvancedParameter
#' @importFrom osp.snapshots create_individual

Copilot AI Feb 24, 2026

Copy link

Choose a reason for hiding this comment

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

These @importFrom osp.snapshots ... entries appear unused in the package code (AdvancedParameter and create_individual are not referenced anywhere under R/). Keeping unused imports can trigger R CMD check notes and makes the dependency surface larger than necessary. Consider removing the unused imports (and the matching NAMESPACE entries) or using them via unqualified calls if they’re intended to be part of the public API.

Suggested change
#' @importFrom osp.snapshots AdvancedParameter
#' @importFrom osp.snapshots create_individual

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

They are not used but I want them to be usable by the user directly.

Comment thread R/formulation.R Outdated
Comment thread man/create_formulation.Rd Outdated
PavelBal and others added 5 commits February 24, 2026 10:50
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* pretty print

* add pretty print and comparison print

* add function to automatically aggregate pop data

* improve

* sort by IndividualID before pivoting. To ensure Id are in the same order when calculating the ration in ctsApp

* improve and add test and redocument

* redoc

* fix test

* fix ci

* fix ci

* update test

* style

* improve coverage

* add new functions to vignettes

* Remove IndividualId column for pretty_pk and compare_pk
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Merge remote-tracking branch 'origin/main' into add-default-outputs

# Conflicts:
#	tests/testthat/_snaps/run-pk-analysis.md
#	tests/testthat/test-run-pk-analysis.R
@PavelBal
PavelBal merged commit 2975b4d into main Feb 24, 2026
4 of 5 checks passed
@PavelBal
PavelBal deleted the add-default-outputs branch February 24, 2026 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

4 participants