Skip to content

Support nimble2 (with conflicts resolved) - #58

Merged
paciorek merged 28 commits into
masterfrom
support-nimble2-resolve-conflicts
Oct 13, 2026
Merged

paciorek merged 28 commits into
masterfrom
support-nimble2-resolve-conflicts

Conversation

@perrydv

@perrydv perrydv commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

@paciorek Here is a PR with all the work to support nimble2 and with merge conflicts resolved.

perrydv added 24 commits June 22, 2026 14:04
@perrydv

perrydv commented Oct 9, 2026

Copy link
Copy Markdown
Contributor Author

@paciorek This turns out to be a bit tricky. I fixed some problems like changing my code from using type to instrType in instrClass objects to catch up to your changes. Now I have test failures that don't obviously have to do with my contributions but I suspect come from some branch/merge glitching. Could you look at line 1616 of test-nimbleModel and line 455 of test-setupMargNodes?

@paciorek

Copy link
Copy Markdown
Contributor

Ok, I am working my way through the test failures. I'll report on them individually here.

Initial side note: When I build the package from support-nimble2-resolve-conflicts, I see this

Warning in utils::tar(filepath, pkgname, compression = compression, compression_level = 9L,  :
  storing paths of more than 100 bytes is not portable:
  ‘nimbleModel/inst/include/nimbleModel/predef/modelValuesBase_nClass/modelValuesBase_nClass_copyFiles.txt’

@paciorek

paciorek commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

The test failure in test-setupMargNodes is from line 455, in particular the 3rd declaration, the declaration of y. It's happening because there is a (new, compared to master branch) element of varInfo that has name "NA". That element seems to appear because in make_modelClass_from_nimbleModel, in decl_methods |> lapply(\(x) all.vars(body(x))) , "i" is appearing as a variable related to getParam_one, and of course i is not in any varInfo:

$getParam_one
[1] "paramID_"                                            
[2] "LocalAns_"                                           
[3] "model"                                               
[4] "y"                                                   
[5] "idx"                                                 
[6] "lifted_b1_times_x_oBi_cB_plus_mu_oBi_cB_L2"          
[7] "lifted_exp_oPb2_times_x_oBi_cB_plus_mu2_oBi_cB_cP_L2"
[8] "i"                                                   

So I think this one does go back to changes you've made.

[EDIT] It looks like the test-nimbleModel.R line 1616 issue is the same thing. i has crept into declVars in the processing of the second declaration in make_modelClass_from_nimbleModel:

Browse[1]> declVars
[1] "y"                                                                          
[2] "n1"                                                                         
[3] "n2"                                                                         
[4] "mu"                                                                         
[5] "lifted_chol_oPpr_oBn1_oBi_cBton2_oBi_cB_comma_n1_oBi_cBton2_oBi_cB_cB_cP_L2"
[6] "logProb_y"                                                                  
[7] "pr"                                                                         
[8] "i"                                                                          

@perrydv

perrydv commented Oct 13, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the help @paciorek. After dealing with a bunch of issues, this should pass tests on this run or soon. The last issue was just changing expect_identical to expect_equal in some spots, for ubuntu failures that pass on MacOS. So I'd say it's ready for any code review you want to do.

@paciorek

Copy link
Copy Markdown
Contributor

Ok, I am merging in. At some point, I'd like to better understand the setup for getParam, but I'm not looking deeply into it now.

@paciorek
paciorek merged commit 893fc77 into master Oct 13, 2026
4 checks passed
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.

2 participants