Multipop calcPopValue and updates

[darizasu]

This PR adds new functionality to the MultiPop class and updates flattenMultiPop and calcPopValue.

Summary of changes

• Add splitPop, which makes it easier to create flat or nested MultiPop objects from grouping specifications. Grouping specs are passed through the by argument as atomic vectors or functions returning such vectors. If by is a list, each element is applied recursively to build nested MultiPop objects.

• Update flattenMultiPop to better preserve names in nested structures. A new argument, preserveNames, controls how names are handled when flattening:

  • "auto" (default): keep names only if they are all present and remain unique after flattening; otherwise drop them.
  • "concatenate": prefix child names with parent names using underscores. Names are kept only if the resulting values are unique.
  • "force": always keep concatenated names and make them unique with make.unique() if needed, with a warning.
  • "none": drop all names.

  The default "auto" mode is conservative: names are preserved only when they remain meaningful. Use "concatenate" to build hierarchical names when possible, "force" to guarantee unique names, and "none" to explicitly discard names.

• Update calcPopValue to a more general form controlled by a user-provided FUN function. calcPopValue is now exported and also gains a simplify argument, similar to sapply() and related functions. This allows the output to be simplified to a requested nesting level, and the simplification is handled by flattenMultiPop.

  To avoid losing track of the original structure that produced the simplified output, each resulting matrix now carries a "source" attribute. This is a data frame with one column per nesting level in the original object and one row per row in the simplified output matrix. Each entry records the name (character) or index (integer if no name available) of the parent node at that level.

• Update selectPop to use the new version of calcPopValue.

Commit overview

• 19296fd
  Adds the exported splitPop function and the internal .splitAtLevels and .splitPop helpers, along with documentation and tests.

• 10583df
  Updates flattenMultiPop to include the preserveNames argument. Documentation and tests were updated as well.

• af2a49e
  Updates calcPopValue to support a simplify argument. To build the new "source" attribute, this commit adds the internal helpers .collectLeafPaths and .formatCalcPopSource. Documentation and tests were also updated.

• 0827426
  Updates selectPop to use the new version of calcPopValue. Documentation and tests were also updated.

• 9c298a3
  Fixes a bug where the "source" attribute and the simplified matrix had different numbers of rows.

• 53b14dc
  Changes automatically introduced by GitHub actions after pushing to origin.

Discussion topics

Please let me know your thoughts about:

• How splitPop builds nested MultiPop objects from a list of functions.
• The new preserveNames argument in flattenMultiPop, particularly on whether the default behavior ("auto") feels sensible.
• For calcPopValue, its overall design and implementation.
  • What are your thoughts on its current default values (simplify=FALSE, level=1)? This implies that, whenever simplify=TRUE, the input will be flattened to level=1 (unless the user specifies other level), producing a single matrix for the entire structure. A second option would be to calculate the input's max depth (with .depthMultiPop) which would produce a matrix for each pop arranged in a list with the same structure as the input x. A third option would be to leave level undefined and expect the user to define it whenever simplify=TRUE, raising an error if they don't.
  • I think calcPopValue could become a useful base function for other “get” functions. For example, we could update pheno (and similarly gv and ebv) to use the new calcPopValue functionality, which would let them work with MultiPop objects:

pheno = function(x, simplify = TRUE, level = 1) {
  calcPopValue(x, FUN = \(x) x@pheno, simplify = simplify, level = level)
}

Similarly, a new phenoPop to calculate pop-level phenotypes can be also implemented to use calcPopValue:

phenoPop = function(x, FUN, simplify = TRUE, level = 1, ...) {
  calcPopValue(x, FUN = \(x) FUN(x@pheno, ...), simplify = simplify, level = level)
}

Other functions in the pullXXX family could likely be updated in a similar way, although they would need a more substantial refactor than pheno:

pullQtlGeno =
  function(x, trait = 1, chr = NULL, asRaw = FALSE, simParam = NULL,
           nThreads = NULL, simplify = TRUE, level = 1) {
    calcPopValue(
      x,
      FUN = # Copy the current `pullQtlGeno` function definition here,
      simplify = simplify,
      level = level
    )
  }

That said, as @gregorgorjanc pointed out, this may blur the line between "getting" values from Pop objects and "calculating" new values based on the contents of each Pop within a MultiPop (see #239 (comment)). It could also feel a bit confusing for users to see a calc function used inside functions that are conceptually about retrieving values.

Related to #239

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 97.68340% with 6 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (MultPop@dc9bb4f). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
R/selection.R	95.90%	5 Missing ⚠️
R/mergePops.R	99.25%	1 Missing ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             MultPop     #287   +/-   ##
==========================================
  Coverage           ?   46.82%           
==========================================
  Files              ?       38           
  Lines              ?    10067           
  Branches           ?        0           
==========================================
  Hits               ?     4714           
  Misses             ?     5353           
  Partials           ?        0           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[gregorgorjanc]

calcPopValue() is becoming a widely used "workhorse" function for much of work with "values" in MultiPop objects. This is great. We initially intended it as a "calculate some summary for each population object in a MultiPop object", so calcPopValue() was an obvious choice name. But now it also used widely to "get just about any value fro each population object in a MultiPop object", making me wonder if it would be better to call this getPopValue() and then what this getter returns depends on its FUN argument.

I am raising this because in our recent meeting I saw calcPopValue() being used over and over to get individual values for each population of a MultiPop object and the term calc was confusing me all the time.

This example in the next line calcPopValue(mp1, FUN = pheno, ...) is a nice example of this - here we are just extracting phenotypes, while with calcPopValue(mp1, FUN = function(x) colMeans(pheno(x)) ...) we are calculating.

This is all just semantics! In most cases users will call functions that internally call this calcPopValue/getPopValue so naming is not critical, but could help us understand what we are doing.

[gregorgorjanc]

Maybe I am overthinking this!?

[gregorgorjanc]

About

Update flattenMultiPop to better preserve names in nested structures. A new argument, preserveNames, controls how names are handled when flattening:

"auto" (default): keep names only if they are all present and remain unique after flattening; otherwise drop them.
"concatenate": prefix child names with parent names using underscores. Names are kept only if the resulting values are unique.
"force": always keep concatenated names and make them unique with make.unique() if needed, with a warning.
"none": drop all names.
The default "auto" mode is conservative: names are preserved only when they remain meaningful. Use "concatenate" to build hierarchical names when possible, "force" to guarantee unique names, and "none" to explicitly discard names.

I think I like your choice of default (auto). What is R's default though in such cases? I think it's force that uses make.unique(), which is always annoying me when it happens (mostly because I have done something wrong!). I am easy here as I have not used this system yet, so I don't have strong feelings.

@gaynorr what are your thoughts?

[gregorgorjanc]

How splitPop builds nested MultiPop objects from a list of functions.

I really like how splitPop() works!

[gregorgorjanc]

For calcPopValue, its overall design and implementation.
What are your thoughts on its current default values (simplify=FALSE, level=1)? This implies that, whenever simplify=TRUE, the input will be flattened to level=1 (unless the user specifies other level), producing a single matrix for the entire structure. A second option would be to calculate the input's max depth (with .depthMultiPop) which would produce a matrix for each pop arranged in a list with the same structure as the input x. A third option would be to leave level undefined and expect the user to define it whenever simplify=TRUE, raising an error if they don't.

I like the chosen defaults calcPopValue(..., simplify=FALSE, level=1) because it returns an object that has the same structure as input (so like what lapply() does). However, if I do want to simplify then I get the thing that most people will want - one matrix. I think this is a good balance.

Arguably simplify=TRUE might be a good default too. I don't have strong feelings here. I reckon that longer-term use will show what is more useful. Perhaps most use cases will be to get a single matrix, arguing for simplify=TRUE, but maybe not. Hard to judge at this time.

@gaynorr what are your thoughts on this?

[gregorgorjanc]

I think calcPopValue could become a useful base function for other “get” functions. For example, we could update pheno (and similarly gv and ebv) to use the new calcPopValue functionality, which would let them work with MultiPop objects:

I reviewed this PR with some time-distance and jumped straight into the code, so interesting that I commented up on the blur between the calcX and getX. I feel like naming this function as getPopValue() might now be more appropriate.

[gregorgorjanc]

Minor bits left to iron out, but this is close to getting merged. Would be good to get your feedback to @gaynorr!

[gregorgorjanc]

Revise the doc in relation to the calcX() vs getX() discussion point below.