Merge in updated redist #197

philipwosull · 2025-07-14T04:42:31Z

Adding new sampling spaces, splitting methods, and smc weights among other things

…ttle more to be done

…be incorrectly) tracked

…lated matrices

…pose splitting actions to R

…ons and added more stuff to splitting file

…d redist_optimal_gsmc

…plit

… plans

…ead of preemptively

…constraints

christopherkenny

This is much closer. I've added more comments and some existing ones were not yet resolved. Thanks Philip.

christopherkenny · 2025-08-08T18:56:43Z

R/confint.R

+                std_err <- sd(run_means) / sqrt(max(chain) - 1) # be slightly conservative
+            }
+    }else{
+        cli::cli_abort("Can't do non-coda std errors for single MCMC chain!")


This should be {.pkg coda}.

christopherkenny · 2025-08-08T18:58:45Z

R/diagnostics.R

+    # ignore if not a supported algorithm
+    if(!algo %in% summary_supported_algs){
+        cli::cli_abort("{.fn summary} is not supported for the {toupper(algo)} algorithm.")
+        return(invisible(1))


This return line doesn't ever get reached because it is after an abort.

christopherkenny · 2025-08-08T19:00:37Z

R/diagnostics.R

+        all_splitting_methods <- sapply(all_run_info, function(x) x$split_method)
+        if(length(unique(all_splitting_methods)) != 1){
+            cli::cli_abort("{.fn summary} is not supported for plans sampled using different splitting methods")
+            return(invisible(1))


Same for this return.

christopherkenny · 2025-08-08T19:00:47Z

R/diagnostics.R

+            all_forward_kernel_params[[i]]$cut_k_used <- NULL
+            if(!identical(all_forward_kernel_params[[1]], all_forward_kernel_params[[i]])){
+                cli::cli_abort("{.fn summary} is not supported for plans sampled using different splitting parameters")
+                return(invisible(1))


And this one.

christopherkenny · 2025-08-08T19:14:48Z

R/redist_map.R


    existing_col <- names(tidyselect::eval_select(rlang::enquo(existing_plan), x))
-    if (length(existing_col) == 0)
+    if (length(existing_col) == 0){


Are we just catching NULL or is this related to new features?

christopherkenny · 2025-08-08T19:21:45Z

R/redist_constr.R

+#' @param fn A function
+#' @rdname constraints
+#' @export
+add_constr_custom_plan <- function(


Wait what's the difference between these two custom constraints now?

christopherkenny · 2025-08-08T19:27:16Z

R/redist_ms.R

+      silent = silent
+    )
+
+    sampled_inidices <- sample.int(n = n_smc_nsims, size = chains, replace = F)


christopherkenny · 2025-08-08T19:32:18Z

R/redist_ms_parallel.R

+        control = list(),
+        adapt_k_thresh = .99) {
+
+    cli::cli_abort("redist_mergesplit_parallel is deprecated. Please call redist_mergesplit now.")


This should just be forwarding to the new redist_mergesplit with the special .Deprecated

christopherkenny · 2025-08-08T19:33:56Z

R/redist_plans.R

 # plans has n_precinct columns and n_sims rows
 # map is a redist_map
-# algorithm is one of "smc" or "mcmc"
+# algorithm is one of "smc" or "mcmc" or "smc_ms


missing "

christopherkenny · 2025-08-08T19:46:24Z

src/cpp_code_overview.md

@@ -0,0 +1,29 @@
+# C++ Redist Code Overview 


Does this file need to be added to the .Rbuildignore?

CoryMcCartan

@philipwosull when you run the formatter you have to match our existing formatting! 4-space indents among other things. That created a lot of diff noise that makes it hard to tell what substantively changed.

what is the status of devtools::check() at this point?

DESCRIPTION

CoryMcCartan · 2025-08-10T18:01:52Z

R/RcppExports.R

    .Call(`_redist_calcPWDh`, x)
 }

+#'


do we need to export these from c++ to R if they're not used by any package R code?

if we are going to keep them exported (f,rom c++) but internal, then I'd like to change the names, because people can still see these functions with ::: and the names are super verbose

CoryMcCartan · 2025-08-10T18:03:22Z

R/confint.R

-    std_err <- summary(mcmc)$statistics["Time-series SE"]
-    if (isTRUE(by_chain)) {
-        std_err <- std_err * sqrt(max(chain))
+    if(use_coda){


We are also back to bad code formatting. Please fix the formatting throughout.

CoryMcCartan · 2025-08-10T18:04:53Z

R/diagnostics.R

the new summary() is incredibly long. Can we break this up into multiple functions? Not a blocker for internal use but I really worry about long-term maintainability

CoryMcCartan · 2025-08-10T18:05:20Z

R/plan_calculations.R

This is still in this PR

CoryMcCartan · 2025-08-10T18:22:08Z

R/redist_smc.R

-#' # One run with multiple cores
-#' redist_smc(fl_map, 1000, ncores = 2)


this is meant to demonstrate ncores not nproc. Add another example if you really want that, though I don't think we do want taht

CoryMcCartan · 2025-08-10T18:22:25Z

R/redist_smc.R

+        weight_type = weight_type,
+        nproc = nproc,
+        ncores = ncores,
+        custom_size_split_list = list(),


@philipwosull ?

CoryMcCartan · 2025-08-10T18:23:12Z

README.Rmd


 Papers:

+- O'Sullivan, P., McCartan, C., & Imai, K. (Working Paper Forthcoming). Generalized Sequential Monte Carlo Sampling for Larger-scale Redistricting Problems.


(in preparation) would be the normal way to refer to this

CoryMcCartan · 2025-08-10T18:23:51Z

README.md

+#> Starting Chain 1
 #> SEQUENTIAL MONTE CARLO
-#> Sampling 500 99-unit maps with 4 districts and population between 760,827 and 762,350.
+#> Using Graph Sampling space to sample 500 99-unit maps with 4 districts and population between 760827 and 762350.


@philipwosull what did you find here? is it something to do with the format string?

CoryMcCartan · 2025-08-10T18:26:34Z

R/redist_smc.R

+  if (!missing(truncate)) {
+    cli::cli_abort("{.arg truncate} is deprecated. Do not use this argument.")
+  }
+  if (!missing(trunc_fn)) {
+    cli::cli_abort("{.arg truncate} is deprecated. Do not use this argument.")
+  }
+  if (!missing(final_infl)) {
+    cli::cli_abort("{.arg truncate} is deprecated. Do not use this argument.")
+  }


If these are hard deprecations then we should just remove them from the argument list!

@christopherkenny I am fine with breaking changes for these args

CoryMcCartan · 2025-08-10T18:43:39Z

@philipwosull when you get a chance, merge in main again which will hopefully get R CMD CHECK automatically running on here every time you push

…re generated fresh for each function call

philipwosull added 30 commits July 19, 2024 17:11

small tweak to readme

0e6b04e

adding code for gsmc method

2deabd8

adding some files I forgot to push

4e506ae

added documentation for new functions. Mostly finished but still a li…

5bfd0d8

…ttle more to be done

removing format library to try to fix compilation issue on FAS cluster

1baad5b

added diagnostic flag to make high mem usage optional

1ae29ca

fixed sd_lp issue for gsmc in summary

4bb7583

refactored Plan object code to drastically reduce memory usage

0e579eb

updated some function doc pages

eb23a5b

Weights now calculated in seperate function and parent tries are (may…

2be4d4f

…be incorrectly) tracked

renamed parent tries mat to parent unsuccessful tries mat

fa2e5ca

added population tempering

466fd95

can now pass vector of k values in instead of one for entire run

a053c70

added basic version of original smc with optimal weights

400afa0

added basic version of original smc with optimal weights

37b81c7

added district counting functions

a002c5b

basic smc no longer stores useless info

d652bb2

takes into account index changes for final resampling for ancestor re…

73ce6fc

…lated matrices

added task tracker document

26fba06

added new file for smc_withMCMC steps code

3dd1bc7

updated task tracking doc

0d2c678

created and tested seperate plan update function and added code to ex…

a30e4ec

…pose splitting actions to R

put splitting functions in their own file

b24e115

reorganized file structure. Added seperate files for weight computati…

369afb2

…ons and added more stuff to splitting file

created all purpose code to handle smc and gsmc with optimal weights

e4c7b57

removed old gsmc and basic_smc files, now replaced by new consolidate…

3c8c4a9

…d redist_optimal_gsmc

refactored code so R output now orders region ids by when they were s…

8143211

…plit

tweaked find edge to cut function to better integrate with merge split

4d1e88f

more tweaks to the updating functions from a cut tree

4906982

started working on merge split stuff

8feac8b

philipwosull added 14 commits July 25, 2025 15:58

for optimal weights moved linking edge computation from weights

90c1bbb

flipped linking edge term ratio in optimal weights. fixed

4f5daa8

added and tested faster method to convert log linking edge for merged…

cd8799b

… plans

now only compute linking edge selection probability when needed, inst…

7b07488

…ead of preemptively

fixed issue with district populations not being resampled correctly

f68a826

when inferring seat sizes only warns about not-tight bounds once

a7e6a79

added mergesplit parallel back with deprecation warning

d5d7f83

max_dev now works for multimember plans

8dd1677

more code tweaks

93e6c3d

more mostly style changes for pr

69a4b66

added thresholding in constraints and new test for custom plan based …

70fdd1b

…constraints

adding some missing man pages

d70da95

made summary backwards compatible with previous redist plans

b4f5b28

moved constants to package file

90dfcaa

christopherkenny requested review from CoryMcCartan and christopherkenny August 8, 2025 18:52

christopherkenny reviewed Aug 8, 2025

View reviewed changes

philipwosull added 2 commits August 8, 2025 19:51

modified constraints cpp code to reduce memory usage

4472305

thresholding now turns constraints into indicators

e71879f

CoryMcCartan requested changes Aug 10, 2025

View reviewed changes

philipwosull added 9 commits August 21, 2025 11:45

added missing array header

01da894

can now exactly specify what number of regions plan constraints apply to

69d2972

fixed problem with non-unique thread ids

010d185

fixed remaining thread id assignment issues. Now ensures thread ids a…

3b0ea35

…re generated fresh for each function call

fixed redist parity bug with MMD

32eabe2

renaming some mergesplit parameters

76bf3a7

removing stale debugging statement

a48ba78

forgot to rename some ms move to step in diagnostics

770fb00

updated diagnostics to return rhats and diagnostics df

a207d6e

		#' # One run with multiple cores
		#' redist_smc(fl_map, 1000, ncores = 2)


		Papers:

		- O'Sullivan, P., McCartan, C., & Imai, K. (Working Paper Forthcoming). Generalized Sequential Monte Carlo Sampling for Larger-scale Redistricting Problems.

Merge in updated redist #197

Are you sure you want to change the base?

Merge in updated redist #197

Uh oh!

Conversation

philipwosull commented Jul 14, 2025

Uh oh!

christopherkenny left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CoryMcCartan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CoryMcCartan commented Aug 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants