Skip to content

Conversation

@kamilsi
Copy link
Contributor

@kamilsi kamilsi commented Oct 1, 2025

What happened:

  • Bug discovery: @aleksandraduda2 identified an issue with unequal allocation ratio handling in minimisation randomization.
  • TDD first: I began with tests, adding 7 failing tests (failures: https://github.com/ttscience/unbiased/actions/runs/18165618890) to reproduce the bug (and added a few more that already passed to improve coverage).
  • Maintenance: Performed routine GitHub Actions housekeeping to keep CI healthy.
  • Fix + housekeeping: Implemented the fix, then did all versioning updates and documentation:
    • bumped package to 1.0.2
    • updated @apiversion, tests, and vignette references
    • added the 1.0.2 entry to NEWS.md
    • committed the changes together with the bugfix.

Summary by CodeRabbit

  • Bug Fixes
    • Fixed minimisation randomization to correctly honor unequal allocation ratios (e.g., 3:1:1).
  • Documentation
    • Updated release notes and references for version 1.0.2.
    • Enhanced API documentation metadata with new tags for easier navigation.
  • Tests
    • Added end-to-end and unit tests validating ratio adherence, deterministic behavior, scaling invariance, and method variability.
  • Chores
    • Updated CI to use environment restoration, pinned R 4.2.3, improved artifact upload, and refined linting.
    • Minor editor settings adjustments and version bumps across metadata.

…andomization.

Also added tests for weights and method (which pass).
@kamilsi kamilsi linked an issue Oct 1, 2025 that may be closed by this pull request
@kamilsi kamilsi changed the title Added tests (that currently fail) to capture bug with unequal ratio r… Bugfix: unequal allocation Oct 1, 2025
@kamilsi kamilsi requested review from lwalejko and salatak October 1, 2025 15:01
@kamilsi kamilsi merged commit c8c7ea4 into main Oct 2, 2025
4 checks passed
@kamilsi kamilsi deleted the 106-validate-minimization-randomization-with-unequal-allocation-ratios branch October 2, 2025 14:23
@kamilsi kamilsi mentioned this pull request Oct 2, 2025
kamilsi added a commit that referenced this pull request Oct 2, 2025
- Add font libraries (libfontconfig1-dev, libfreetype6-dev, etc.) for systemfonts
- Add image libraries (libpng-dev, libjpeg-dev, libtiff5-dev, etc.) for ragg
- Add PKG_CONFIG_PATH export for lint workflow
- Fixes CI failures on all workflows after PR #107 merge
@kamilsi kamilsi mentioned this pull request Oct 2, 2025
@kamilsi
Copy link
Contributor Author

kamilsi commented Oct 7, 2025

@coderabbitai review

@coderabbitai
Copy link

coderabbitai bot commented Oct 7, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai
Copy link

coderabbitai bot commented Oct 7, 2025

Caution

Review failed

The pull request is closed.

Walkthrough

CI workflows switch from setup-r-dependencies to setup-renv with explicit R 4.2.3, plus ad-hoc installs for needed R packages. A bug fix updates imbalance calculation in randomize_minimisation_pocock. Version bumped to 1.0.2 with NEWS, API annotation, and references updated. Additional tests validate ratio adherence and error handling.

Changes

Cohort / File(s) Summary
CI workflows (renv migration, explicit R 4.2.3)
.github/workflows/document.yaml, .github/workflows/lint.yaml, .github/workflows/pkgdown.yaml, .github/workflows/test-coverage.yaml
Replace setup-r-dependencies with setup-renv; pin R to 4.2.3; add on-demand installs for roxygen2/lintr/cyclocomp/pkgdown/covr; set Rscript shells; upgrade upload-artifact v3→v4 (coverage).
Developer tooling and lint config
.lintr, .vscode/settings.json
Update lintr config: enable cyclocomp and line length, disable several linters, add exclusions for vignettes/; set VSCode r.lsp.promptToInstall to false.
Versioning and metadata
DESCRIPTION, NEWS.md, inst/plumber/.../plumber.R, vignettes/articles/references.bib
Bump to 1.0.2; add NEWS entry describing minimisation ratio bug fix; update plumber API version to 1.0.2 with new tags (read, other); update BibTeX version note.
Core algorithm fix
R/randomize-minimisation-pocock.R
In imbalance calculation, change operation from multiply by ratio[arm] to divide by ratio[arm].
Tests (unit and E2E)
tests/testthat/test-randomize-minimisation-pocock.R, tests/testthat/test-E2E-study-minimisation-pocock.R, tests/testthat/test-error-handling.R
Add comprehensive tests for ratio adherence, determinism, scaling invariance, method variability, and E2E 3:1:1 allocation; update SENTRY_RELEASE expectation to 1.0.2.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Client
  participant API as Plumber API
  participant Core as randomize_minimisation_pocock()
  participant State as Study State

  Client->>API: POST /randomize (covariates, ratio, p)
  API->>State: Read current allocations
  API->>Core: Compute next arm (state, ratio, p, method)
  note over Core: Imbalance computation uses division by ratio[arm]
  Core-->>API: Selected arm
  API-->>State: Persist assignment
  API-->>Client: 200 OK (assigned arm)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

I thump my paws at version two,
Ratios right, the math anew.
Pipelines hop through renv’s gate,
Linters sniff, then validate.
Tests now nibble every case—
3:1:1 falls into place.
Carrot-merge complete; ship with grace. 🥕✨

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch 106-validate-minimization-randomization-with-unequal-allocation-ratios

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f570d20 and 951f8f3.

📒 Files selected for processing (14)
  • .github/workflows/document.yaml (1 hunks)
  • .github/workflows/lint.yaml (1 hunks)
  • .github/workflows/pkgdown.yaml (1 hunks)
  • .github/workflows/test-coverage.yaml (2 hunks)
  • .lintr (1 hunks)
  • .vscode/settings.json (1 hunks)
  • DESCRIPTION (1 hunks)
  • NEWS.md (1 hunks)
  • R/randomize-minimisation-pocock.R (1 hunks)
  • inst/plumber/unbiased_api/plumber.R (1 hunks)
  • tests/testthat/test-E2E-study-minimisation-pocock.R (1 hunks)
  • tests/testthat/test-error-handling.R (2 hunks)
  • tests/testthat/test-randomize-minimisation-pocock.R (1 hunks)
  • vignettes/articles/references.bib (1 hunks)

Comment @coderabbitai help to get the list of available commands and usage tips.

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.

Validate minimization randomization with unequal allocation ratios

2 participants