Add partition spec and index layout infrastructure for sex strata - #1581
Add partition spec and index layout infrastructure for sex strata#1581szu-yun-ko wants to merge 16 commits into
Conversation
🎨 Chore: code formatting workflowOur automated workflows cannot run on forks because of permission issues, and thus, we ask that you run the following code locally and push any changes that are created to your feature branch. You will only be reminded of this once per PR. Thank you! Format C++ code
Format R code
styler::style_pkg() # Style R code
roxygen2::roxygenise() # Update documentation
styler::style_pkg() # Style R code again
roxygen2::roxygenise() # Update documentation again
usethis::use_tidy_description() # Style DESCRIPTION filePush changes
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1581 +/- ##
==========================================
+ Coverage 84.88% 86.21% +1.33%
==========================================
Files 105 59 -46
Lines 9473 2684 -6789
Branches 536 628 +92
==========================================
- Hits 8041 2314 -5727
+ Misses 1395 258 -1137
- Partials 37 112 +75 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@awilnoaa @nathanvaughan-NOAA @Andrea-Havron-NOAA This PR should be ready for an initial review. This PR addresses the tasks mentioned in issue #1557, after the review comments are addressed, I will continue tackling tasks in issue #1587 in this same PR. Partition representation:
Wiring into population:
Partitioned derived-quantity storage:
Tests added:GTest
testthat
|
7a0195e to
e8169b0
Compare
There was a problem hiding this comment.
Pull request overview
Adds the first layer of partition/indexing infrastructure to support sex strata in population dynamics (as groundwork for future partitioned state/derived quantities), and exposes the new stratum dimension through the R output reshaping and related tests.
Changes:
- Introduces partition primitives (
Axis,GroupSelector,PartitionSpec) and indexing helpers (IndexLayout) plus a default sex partition spec. - Stores
partition_specandindex_layoutonPopulationand initializes default sex partition/index layout inCatchAtAge::Initialize(). - Adds partitioned fleet derived-quantity containers (storage only), updates R output dimension naming (
stratum_i), and updates/extends gtests + testthat snapshots.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/testthat/test-get_estimates.R | Updates expected estimate columns to include stratum_i. |
| tests/testthat/_snaps/get_estimates.md | Updates snapshot output for new column and row/col counts. |
| tests/gtest/test_population_test_fixture.hpp | Initializes partitioned fleet age-based derived quantity vectors in fixtures. |
| tests/gtest/test_population_Subpopulation.cpp | Adds unit tests for PartitionSpec and IndexLayout. |
| tests/gtest/test_population_CatchAtAge_InitializePartition.cpp | Adds fixture-based test for default partition/index layout initialization in CatchAtAge::Initialize(). |
| tests/gtest/test_fleet_Fleet_InitializePrepare.cpp | Extends fleet DQ size/reset expectations to include partitioned vectors. |
| tests/gtest/CMakeLists.txt | Registers the two new gtest executables. |
| R/reshape_output.R | Maps n_strata dimension header to stratum_i for tidy output. |
| inst/include/population_dynamics/population/subpopulation.hpp | Implements partition spec structures, indexing layout, and default sex partition helper. |
| inst/include/population_dynamics/population/population.hpp | Adds partition_spec and index_layout members to Population. |
| inst/include/models/functors/catch_at_age.hpp | Initializes default sex partition spec and index layout during model initialization. |
| inst/include/interface/rcpp/rcpp_objects/rcpp_models.hpp | Adds partitioned fleet DQ vectors + dimension metadata for R interface output. |
Comments suppressed due to low confidence (1)
inst/include/interface/rcpp/rcpp_objects/rcpp_models.hpp:1276
index_weight_at_age(and itsDimensionInfo) are initialized a second time immediately afterindex_weight_at_age_by_partition. This duplicate block is redundant and likely accidental; it should be removed to avoid confusion and to ensure future edits don’t update only one copy.
derived_quantities["index_weight_at_age"] = fims::Vector<Type>(
fleet_interface->n_years.get() * fleet_interface->n_ages.get());
derived_quantities_dim_info["index_weight_at_age"] =
fims_popdy::DimensionInfo(
"index_weight_at_age",
364d010 to
a7e0ab9
Compare
|
This PR already adds partition structure (
// Pooled write (unchanged)
const Type pooled = (Fmort * f_multiplier * selectivity) / Z
* N_aa * (1 - exp(-Z));
fdq_["landings_numbers_at_age"][i_age_year] += pooled;
// Partitioned write (future; sex-only Model 1)
const auto split_factors = SexStratumSplitFactors(
population->partition_spec,
population->proportion_female.get_force_scalar(age));
for (size_t s = 0; s < population->partition_spec.n_strata(); ++s) {
const size_t i_part =
population->index_layout.i_stratum_age_year(s, year, age);
fdq_["landings_numbers_at_age_by_partition"][i_part] +=
pooled * population->partition_spec.stratum_split_factor(s, split_factors);
}The changes in this PR so far addresses all tasks mentioned in issue #1557 and I'll proceed to work on #1587 after the current work is reviewed. |
| &derived_quantities["landings_numbers_at_age"]; | ||
|
|
||
| // partitioned landings and index at age (storage only; not filled yet) | ||
| derived_quantities["landings_numbers_at_age_by_partition"] = |
There was a problem hiding this comment.
It doesn't look like any of these partitioned derived quantities are being pushed back to info->variable_map. In order to do that I think they will also need to be initialized as VariableVectors in the fleet and population .hpp files. You could look at how their respective non-partitioned quantities are handled to figure out which files are appropriate.
| * @details Pooled quantities use i_age_year(). Partitioned quantities add | ||
| * stratum as the leading dimension via i_stratum_age_year(). | ||
| */ | ||
| struct IndexLayout { |
There was a problem hiding this comment.
The code here looks to be limited to a single new axis layer with hard coded years/ages axes. Do you have a plan for extending this to nested axes such as sex and area?
There was a problem hiding this comment.
Great question! Multi-axis nesting (e.g. sex × area) is handled by PartitionSpec, not by nesting more dimensions into IndexLayout. PartitionSpec holds a vector of axes, and n_strata() is the product of each axis’s level count (ref. subpopulation.hpp around line 47). Adding area (or season) increases n_strata and it does not require nested helpers like i_sex_area_year_age. The current wiring pulls n_strata from the spec in CatchAtAge::Initialize() (ref. catch_at_age.hpp around line 160):
this->populations[p]->partition_spec = MakeDefaultSexPartitionSpec();
this->populations[p]->index_layout.n_years = this->populations[p]->n_years;
this->populations[p]->index_layout.n_ages = this->populations[p]->n_ages;
this->populations[p]->index_layout.n_strata =
this->populations[p]->partition_spec.n_strata();Keep PartitionSpec axis-agnostic with stratum_split_factor() for precomputed per-stratum weights. Add SexStratumSplitFactors() as a companion to MakeDefaultSexPartitionSpec() so proportion_female is supplied at evaluation time, not stored on the spec.
Map n_strata to stratum_i in reshape_output and update snapshots for partitioned fleet derived quantities on top of current main.
Mirror the pooled landings/index at-age pattern so *_by_partition derived quantities have FleetInterface VariableVectors, Rcpp fields, and info->variable_map entries.
a7e0ab9 to
8c4ca3d
Compare
Represent demand as axis → level-label selections (empty = pooled) so sex-only and future multi-axis configs share one mental model.
Initialize() sets partition_demand to MakePooledPartitionDemand() so populations start with no partitioned strata requested.
|
@szu-yun-ko, @nathanvaughan-NOAA will be out for three weeks so I will be taking over primary review responsibilities while he is gone. I noticed you recently pushed changes to this PR. Is this ready for review now, or are you planning to add more commits first? |
|
@Andrea-Havron-NOAA No problem, thanks for letting me know. The current pushed commits are mainly for addressing the previous review comments. I’ve made more changes locally that addresses tasks mentioned in issue #1587, but I’m still making some final tests and fixes. It will be pushed for review in a few days! Thanks for checking in! |
Keep pooled writes unchanged and split into *_by_partition via WritePartitionedQuantityAtAge when partition_demand is non-pooled.
|
@Andrea-Havron-NOAA @awilnoaa I've pushed recent changes that mainly address issue #1587. The only part left would be the R-interface, but I thought we could make sure the backend behaves as we intended before adding the interface. As the PR is getting larger, below is a brief description of every commit, mainly why we need the changes and how the changes were implemented. Please let me know if anything else is needed during the review process. 1.
|
| } | ||
|
|
||
| this->populations[p]->partition_spec = MakeDefaultSexPartitionSpec(); | ||
| this->populations[p]->partition_demand = MakePooledPartitionDemand(); |
There was a problem hiding this comment.
This looks great. When you get to the user-facing choice, it may be helpful to include a test showing that a user request for partitioned output is applied during model setup and is not accidentally reset to pooled during initialization.
| &derived_quantities_dim_info = | ||
| model->GetFleetDimensionInfo(fleet_interface->id); | ||
|
|
||
| const size_t n_strata = fims_popdy::MakeDefaultSexPartitionSpec().n_strata(); |
There was a problem hiding this comment.
I know the design mentions sex is the only axis for now. This may be outside the scope of this PR, but I am curious, @Andrea-Havron-NOAA: would the DQ storage eventually need to be sized from the active PartitionSpec so it stays in sync with n_strata changes?
There was a problem hiding this comment.
Yes, I think this will need to be made more generic for keeping dimensions in sync. @szu-yun-ko, for the longer term design, will we need a PartitionSpec defined for each partitioning scheme (e.g. AreaPartitionSpec, SeasonPartitionSpec, etc.), or can this be a single generic PartitionSpec build from user-provided axes and levels?
| for (size_t axis_index = 0; axis_index < spec.axes.size(); ++axis_index) { | ||
| const Axis &axis = spec.axes[axis_index]; | ||
| const AxisLevelSelection *selection = nullptr; | ||
| for (const AxisLevelSelection &candidate : demand.selections) { |
There was a problem hiding this comment.
This looks good to me. When the R interface is added, it may be helpful to make sure users cannot accidentally give two different requests for the same axis, like sex = female and sex = male as separate entries. That way the backend always gets one clear request and does not have to silently choose one.
awilnoaa
left a comment
There was a problem hiding this comment.
Looks good overall! I had a few comments/questions but most can be addressed when the user-facing side is added.
| fdq_["index_numbers_at_age_by_partition"], index_naa, | ||
| population->partition_spec, population->index_layout, | ||
| population->partition_demand, | ||
| population->proportion_female.get_force_scalar(age), year, age); |
There was a problem hiding this comment.
Nice progress on partitioned DQ wiring. One concern: WritePartitionedQuantityAtAge still hard-codes sex behavior via proportion_female_at_age and SexStratumSplitFactors. Could we make the writer generic by using a split policy (or per-stratum weights) and keep sex as one policy implementation in a future PR? That would let area/season/etc. reuse the same write path without changing core indexing code.
Andrea-Havron-NOAA
left a comment
There was a problem hiding this comment.
Thanks for the PR — this is useful infrastructure for subpopulation partitioning.
My main takeaway is that the structural pieces (PartitionSpec, IndexLayout, partitioned-at-age write path) are now in place, while some allocation logic is still sex-specific (e.g., SexStratumSplitFactors, proportion_female_at_age). This is great incremental progress, and I'm looking forward to follow-up work that generalized split behavior and sets up dimensions based on user input.
What is the feature?
This PR adds the first layer of partition infrastructure for population dynamics. It addresses parts 1–2 of issue #1557 only (representation of sex and indexing layout). Model dynamics, partitioned derived quantities, and likelihood wiring are intentionally deferred to follow-up PRs.
How have you implemented the solution?
subpopulation.hppwithAxis,GroupSelector,PartitionSpec,IndexLayout, andMakeDefaultSexPartitionSpec()(female/male sex axis).partition_specandindex_layoutmembers toPopulation.CatchAtAge::Initialize().CatchAtAge::Initialize().Design Notes
subpopulation.hpp, matching existing documentation that subpopulations represent generic partitions.IndexLayout::i_age_year()preserves the existing pooled (year, age) layout;i_stratum_age_year()adds stratum as the leading dimension for future partitioned containers (n_strata × n_years × n_ages).Does the PR impact any other area of the project, maybe another repo?
Instructions for code reviewer
👋Hello reviewer👋, thank you for taking the time to review this PR!
nit:(for nitpicking) as the comment type. For example,nit:I prefer using adata.frame()instead of amatrixbecause ...This PR is now ready to be merged.Checklist