Conversation
|
One minor comment, please adopt the style used in the rest of the package. I follow more or less the bionconductor style guide: |
|
Mostly I really want a consistent feel from the user and developer perspective. If we follow a consistent style guide then this would be easier to achieve |
Eventually possible right now not possible |
This perhaps should be generalized to the rest of nlmixr? |
|
I'm happy to modify the style. My default style is snake_case, and I needed this pretty quickly for something else. I also thought that there may be significant modifications after considerations for the ways that it works, so I'm guessing there will be some pretty big rework before it's done. I don't see a simple way to generalize the factor handling to the rest of nlmixr2, but I'd be interested to think about it more and hear thoughts of how it could be done. The way that the nlmixr2 A few ideas of how I considered it were:
|
|
FYI, this PR update only changes things around the edges (documentation issues, mainly). I'll go through the coding style after the underlying functionality has been reviewed. |
|
Another thought for generating parameters for a model with a fixed effect per factor level. Should we go all the way down the path of generating orthogonal contrasts for estimation and then back-transforming them? That seems like it would be mathematically the best (in most scenarios), but it gets a lot more cumbersome. Or, maybe that is already sufficiently covered by |
|
Well, all factors are mu referenced so perhaps simply adding them to the
linear model for covariates would be fine. If that is the case , the
initial estimate would be mostly irrelevant.
This speaks to adding my referenced covariates to focei like nonmem....
|
|
I don't understand "all factors are mu referenced". |
|
It would be good to switch to using the reformulas package for formula processing. https://cran.r-project.org/package=reformulas |
|
This is entirely up to you |
|
@mattfidler, I think that this is ready to merge. Do you want to take one more look? |
There was a problem hiding this comment.
Pull Request Overview
This PR implements a formula interface for nlmixr2 that allows algebraic models to be estimated using a simplified formula syntax similar to lme4::nlmer(). The interface enables users to specify models without writing full nlmixr2 functions, making it easier to fit simple algebraic solutions.
Key changes:
- Adds
nlmixrFormula()function with formula parsing and model generation capabilities - Implements automatic parameter expansion for factor variables
- Provides support for mixed-effects models with random effects grouping
Reviewed Changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| R/nlmixrFormula.R | Main implementation with formula parser and model setup functions |
| tests/testthat/test-nlmixrFormula.R | Comprehensive unit tests for formula parsing and parameter expansion |
| man/*.Rd | Documentation files for the new functions and internal helpers |
| vignettes/nlmixrFormula.Rmd | User guide demonstrating the formula interface with examples |
| NAMESPACE | Exports the new nlmixrFormula() function |
| DESCRIPTION | Updates R dependency and adds vignette builder |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
This does look fine to me. I don't exactly follow what I was also trying to see if there was a way to take out |
|
One last question -- do you want to support iov too? |
|
Like the rest of the I have added a Still haven't done anything about IOV |
|
If you are OK with the changes I made, I am OK merging this. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This reverts commit 27f0e66.
|
I like all your changes. Let me add how to use |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
tests/testthat/test-nlmixrFormula.R:1
- The comment is incomplete and ends abruptly with 'so that'. Complete the explanation or remove the incomplete comment.
test_that(".nlmixrFormulaParser breaks the formula up into the correct bits", {
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| #' @param data The data used in the model | ||
| #' @return the interior of the model() | ||
| #' @keywords Internal | ||
| #' @noRd | ||
| #' @author William Denney | ||
| .nlmixrFormulaSetupModel <- function(start, predictor, residualModel, predictorVar="value", data) { |
There was a problem hiding this comment.
The data parameter is included in the function signature but is not documented in the roxygen comment above, and it's not used in the function body. Either remove the unused parameter or document it properly.
| #' @param data The data used in the model | |
| #' @return the interior of the model() | |
| #' @keywords Internal | |
| #' @noRd | |
| #' @author William Denney | |
| .nlmixrFormulaSetupModel <- function(start, predictor, residualModel, predictorVar="value", data) { | |
| #' @return the interior of the model() | |
| #' @keywords Internal | |
| #' @noRd | |
| #' @author William Denney | |
| .nlmixrFormulaSetupModel <- function(start, predictor, residualModel, predictorVar="value") { |
| stopifnot(length(startValue) %in% c(1, length(paramLabel))) | ||
| if (length(startValue) == 1 && !is.ordered(data[[param]])) { | ||
| message("ordering the parameters by factor frequency: ", startName, " with parameter ", param) | ||
| paramLabel <- names(rev(sort(summary(data[[param]])))) |
There was a problem hiding this comment.
[nitpick] This line performs multiple nested operations that could be hard to follow. Consider breaking it into separate steps with descriptive variable names for better readability.
| paramLabel <- names(rev(sort(summary(data[[param]])))) | |
| paramSummary <- summary(data[[param]]) | |
| sortedParamSummary <- sort(paramSummary) | |
| reversedSortedParamSummary <- rev(sortedParamSummary) | |
| paramLabel <- names(reversedSortedParamSummary) |
| # Setup the dataset for nonlinear mixed-effects fitting with different types of | ||
| # parameters. | ||
|
|
||
| # Simulate the equation y = 3*x + 4(when z = 'a') or 6(when z = 'b') with normally-distributed residual error |
There was a problem hiding this comment.
Missing space before parenthesis in '4(when'. Should be '4 (when' for proper formatting.
| # Simulate the equation y = 3*x + 4(when z = 'a') or 6(when z = 'b') with normally-distributed residual error | |
| # Simulate the equation y = 3*x + 4 (when z = 'a') or 6 (when z = 'b') with normally-distributed residual error |
|
Is this ready for review/merging Bill? |
|
Not quite yet. There are some issues with |
Fix #6.
What it does is it implements a formula interface that allows nlmixr2 models to be estimated when all that is required is an algebraic formula. It takes inspiration from
lme4::nlmer()for the way to define the formula to use.The current state of the PR does not have testing, and documentation needs expansion. There are some design decisions that I think are worth vetting, too.
The main design decisions that are questionable to me are:
IDor are other columns also acceptable?