Pathfinder #848
Pathfinder #848
Conversation
update pathfinder branch
|
Thanks for working on this! Regarding the argument validation, it looks like in PathfinderArgs you're using Also (unrelated), some of the errors in the GHA checks are due to #843. So you'll probably just need to run tests locally until that PR is merged. I think @andrjohns is working on it. |
|
We should make sure that the names and behavior for the In that PR: |
|
@SteveBronder I've merged in |
|
@SteveBronder I'm happy to bring this PR over the finish line and sort out any cmdstanr-trickiness, what's left to be done/fixed? |
|
Just a heads up that we just merged #800 (adding laplace method), which might result in a few conflicts |
|
@andrjohns I think this is ready for a first review! I pulled from the sampling method for tests but if you can think of other things I should add I'd be happy to add them |
Amazing, thanks for this! The current check failures are just from the docs needing to be re-generated and a stray test file being included. I'll fixup those and then review later today. Thanks again! |
Codecov Report
@@ Coverage Diff @@
## master #848 +/- ##
==========================================
+ Coverage 88.30% 88.46% +0.15%
==========================================
Files 12 12
Lines 4362 4517 +155
==========================================
+ Hits 3852 3996 +144
- Misses 510 521 +11
📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today! |
SteveBronder
force-pushed
the
feature/pathfinder
branch
from
a0445bf to
3a27f5f
Compare
R/csv.R
Outdated
| @@ -254,7 +254,7 @@ read_cmdstan_csv <- function(files, | |||
| "\"" | |||
| ) | |||
| } else { | |||
| fread_cmd <- paste0("grep -v '^#' --color=never '", output_file, "'") | |||
| fread_cmd <- paste0("grep -v '^#' --color=never '", path.expand(output_file), "'") | |||
There was a problem hiding this comment.
What was this change needed for?
There was a problem hiding this comment.
I'm not sure I'll revert this change
|
|
||
|
|
tests/testthat/testthat-problems.rds
Outdated
There was a problem hiding this comment.
Can you drop this from the PR as well?
There was a problem hiding this comment.
Sure. Odd, idk where this came from?
There was a problem hiding this comment.
Thanks @SteveBronder! And thanks @andrjohns for reviewing. In addition to Andrew's comments I pointed out in one comment that the arguments need to match the cmdstanpy implementation. We're close but there are a few discrepancies. See comment.
R/model.R
Outdated
| tol_rel_grad = NULL, | ||
| tol_param = NULL, | ||
| history_size = NULL, | ||
| num_draws = NULL, |
There was a problem hiding this comment.
We also need to make sure the arguments match with the cmdstanpy implementation. I think currently there are still discrepancies (e.g. num_draws was changed in cmdstanpy). See @WardBrian's comment here: #848 (comment)
There was a problem hiding this comment.
I'll look those over again
|
@WardBrian for
Is |
|
@jgabry do you want to look at this again or is that a stale review? |
|
Sorry for the delay! I’ll try to take a look tomorrow |
There was a problem hiding this comment.
Thanks @SteveBronder for implementing this and @andrjohns for reviewing. This looks great, just a few minor things. First, I added a few comments/suggestions about minor doc things that I failed to notice in my last review (sorry).
The bigger thing I noticed (although maybe I overlooked some discussion about this) is that the number of draws returned seems to be draws - 1 instead of draws:
file <- file.path(cmdstan_path(), "examples/bernoulli/bernoulli.stan")
mod <- cmdstan_model(file)
stan_data <- list(N = 10, y = c(0,1,0,0,0,0,0,0,0,1))
fit <- mod$pathfinder(data = stan_data, draws = 100)
nrow(fit$draws()) # 99 not 100
|
In addition to my previous comments, we should also add something about pathfinder to the vignette, but that doesn't need to be part of this PR. I'll open a separate issue for that. |
|
Thank you for keeping me updated. Please don't forget to add me to the other issue and let me know if I can provide any help.
Best,
lu
…________________________________
From: Jonah Gabry ***@***.***>
Sent: Wednesday, October 25, 2023 11:16 AM
To: stan-dev/cmdstanr ***@***.***>
Cc: Lu Zhang ***@***.***>; Mention ***@***.***>
Subject: Re: [stan-dev/cmdstanr] [WIP] Pathfinder (PR #848)
In addition to my previous comments, we should also add something about pathfinder to the vignette, but that doesn't need to be part of this PR. I'll open a separate issue for that.
—
Reply to this email directly, view it on GitHub<https://urldefense.com/v3/__https://github.com/stan-dev/cmdstanr/pull/848*issuecomment-1779809471__;Iw!!LIr3w8kk_Xxm!tYMLvVzNWs5u23B7xuubQzh3-YuWAGPNICrbuAjbI4Rxk7rx4YnGOMIBT7LK0Qgbsv-EfgJs5x2B5muc1AGqDT4RBg$>, or unsubscribe<https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AFUN2LK2VNQBLNXR3GW22HTYBFJPXAVCNFSM6AAAAAA4TVZIWGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONZZHAYDSNBXGE__;!!LIr3w8kk_Xxm!tYMLvVzNWs5u23B7xuubQzh3-YuWAGPNICrbuAjbI4Rxk7rx4YnGOMIBT7LK0Qgbsv-EfgJs5x2B5muc1AHTLPmiIw$>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Hi Lu! Have you tried out the implementation in this PR? No obligation, but let us know if you have any feedback if you test it. Also which other issue were you referencing that you should be added to? Happy to include you in any discussion you're interested in. Or was that a comment directed at @SteveBronder or someone else other than me who already knows which issue? |
|
Hi Jonah,
Sorry for the delayed response. Do you have an example code for me to test? I installed the latest versions of Stan and cmdstanr, but the Pathfinder function doesn't work on my macbook.
[cid:103c6e88-9885-430a-8fc5-ea0910e6b0c4]
Thanks a lot.
Best,
Lu
…________________________________
From: Jonah Gabry ***@***.***>
Sent: Wednesday, October 25, 2023 11:27 AM
To: stan-dev/cmdstanr ***@***.***>
Cc: Lu Zhang ***@***.***>; Mention ***@***.***>
Subject: Re: [stan-dev/cmdstanr] [WIP] Pathfinder (PR #848)
Thank you for keeping me updated. Please don't forget to add me to the other issue and let me know if I can provide any help. Best, lu
…
Hi Lu! Have you tried out the implementation in this PR? No obligation, but let us know if you have any feedback if you test it. Also which other issue were you referencing that you should be added to? Happy to include you in any discussion you're interested in. Or was that a comment directed at @SteveBronder<https://urldefense.com/v3/__https://github.com/SteveBronder__;!!LIr3w8kk_Xxm!vhUETZ2wm65U3JMuw5HUffj3foEVGHlK_nL-Gq5Wz2bqwJ0T-fPUkZxh20DNaXHK6HPVEPliY9UUIh02M8pVpJDp-Q$> or someone else other than me who already knows which issue?
—
Reply to this email directly, view it on GitHub<https://urldefense.com/v3/__https://github.com/stan-dev/cmdstanr/pull/848*issuecomment-1779825848__;Iw!!LIr3w8kk_Xxm!vhUETZ2wm65U3JMuw5HUffj3foEVGHlK_nL-Gq5Wz2bqwJ0T-fPUkZxh20DNaXHK6HPVEPliY9UUIh02M8pFhJFteQ$>, or unsubscribe<https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AFUN2LPP4FR7DQ4R45P376LYBFKXNAVCNFSM6AAAAAA4TVZIWGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONZZHAZDKOBUHA__;!!LIr3w8kk_Xxm!vhUETZ2wm65U3JMuw5HUffj3foEVGHlK_nL-Gq5Wz2bqwJ0T-fPUkZxh20DNaXHK6HPVEPliY9UUIh02M8rEMJ0oSw$>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
@LuZhangstat because this hasn't been merged yet you would need to install from this branch: # install from feature/pathfinder branch
remotes::install_github("stan-dev/cmdstanr", ref = "feature/pathfinder")
# after that finishes
# simplest example model to test but you can try any model
library("cmdstanr")
file <- file.path(cmdstan_path(), "examples/bernoulli/bernoulli.stan")
mod <- cmdstan_model(file)
stan_data <- list(N = 10, y = c(0,1,0,0,0,0,0,0,0,1))
fit <- mod$pathfinder(data = stan_data) # you can test arguments like num_paths and othersDoes that work? |
|
Yes, it works. I will play with it and let you know if I have any further comments. Thank you so much for all your help!
Best,
Lu
…________________________________
From: Jonah Gabry ***@***.***>
Sent: Wednesday, November 1, 2023 7:49 AM
To: stan-dev/cmdstanr ***@***.***>
Cc: Lu Zhang ***@***.***>; Mention ***@***.***>
Subject: Re: [stan-dev/cmdstanr] [WIP] Pathfinder (PR #848)
@LuZhangstat<https://urldefense.com/v3/__https://github.com/LuZhangstat__;!!LIr3w8kk_Xxm!ptf9YKdUyYHOPXTXVqY-ZxAxbZRkhwWL-G36TAxgj_c4lGpp3pQNLgVQn2Qc3vKpY_wml2Q5P1DzxKu639bbLkb4QQ$> because this hasn't been merged yet you would need to install from this branch:
# install from feature/pathfinder branch
remotes::install_github("stan-dev/cmdstanr", ref = "feature/pathfinder")
# after that finishes
# simplest example model to test but you can try any model
library("cmdstanr")
file <- file.path(cmdstan_path(), "examples/bernoulli/bernoulli.stan")
mod <- cmdstan_model(file)
stan_data <- list(N = 10, y = c(0,1,0,0,0,0,0,0,0,1))
fit <- mod$pathfinder(data = stan_data) # you can test arguments like num_paths and others
Does that work?
—
Reply to this email directly, view it on GitHub<https://urldefense.com/v3/__https://github.com/stan-dev/cmdstanr/pull/848*issuecomment-1789091920__;Iw!!LIr3w8kk_Xxm!ptf9YKdUyYHOPXTXVqY-ZxAxbZRkhwWL-G36TAxgj_c4lGpp3pQNLgVQn2Qc3vKpY_wml2Q5P1DzxKu639aqzqghhg$>, or unsubscribe<https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AFUN2LMNPP4QGKDU4IOMVSDYCJOPPAVCNFSM6AAAAAA4TVZIWGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOBZGA4TCOJSGA__;!!LIr3w8kk_Xxm!ptf9YKdUyYHOPXTXVqY-ZxAxbZRkhwWL-G36TAxgj_c4lGpp3pQNLgVQn2Qc3vKpY_wml2Q5P1DzxKu639aTHPwM5w$>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
Sounds good thanks Lu! |
|
@SteveBronder I have a sec now so I can go ahead and take care of the doc-related comments in my recent review. Can you just see what's up with pathfinder returning |
just reindenting
Ok I just fixed the remaining doc issues so I think the only things left to do before merging this are:
Otherwise I think we're good to go! |
|
Just fixed! |
|
It was a silly error in csv.R |
|
Thanks Steve! |
|
@jgabry good to go? |
|
Good to go! Merging now. |
|
Thanks again for working on this! |
Submission Checklist
Summary
This implements pathfinder in cmdstanr. Opening as a draft as I'm getting an error on the tests I'm not sure how to fix. Where is the validation for each of the arguments done at? I put them all in the validation function and while the model shows up with errors such as the below they do not actually throw an error.
I also still need to fix the docs. Once the above is done it should be ready for review
Copyright and Licensing
Simons Foundation
By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the following licenses: