Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes 975 by only removing leftmost array dimension if equal to 1 #993

Merged
merged 3 commits into from
Jun 8, 2024

Conversation

SteveBronder
Copy link
Collaborator

Submission Checklist

  • Run unit tests
  • Declare copyright holder and agree to license (see below)

Summary

Fixes #975 by removing only the leftmost dimension of the init if it is equal to 1. Previously the code was calling drop() but that drops all dimensions equal to 1. @andrjohns I'd appreciate if you can check my logic here, but I think with a subset of draws we want to drop only the first dimension if it is equal to 1. This passes all the tests I have setup for weird dimension sized parameter types so I think this logic is correct but I'd appreciate another set of eyes.

Copyright and Licensing

Please list the copyright holder for the work you are submitting
(this will be you or your assignee, such as a university or company):
Simons Foundation

By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the following licenses:

@mhollanders would you mind installing the branch of cmdstanr and checking whether this works for the larger model you are working with? I have this passing your minimal test case but would rather be certain I'm not just catching a small edge case

@andrjohns
Copy link
Collaborator

Thanks! I'll sort out the windows tests and review tomorrow

@andrjohns
Copy link
Collaborator

@SteveBronder for some reason the test model only had a single unique lw value at this step in process_init_approx when psis_resample=TRUE - but only on Windows, and only when called in testthat::expect_no_error() (which is all kinds of bizarre)

I've set the test to use psis_resample=FALSE for now, but it might suggest there's some kind of weird interaction/issue with the process on Windows

@andrjohns andrjohns force-pushed the fix/issue-975-inits branch from dfd83c9 to 2345489 Compare June 8, 2024 15:22
Copy link
Collaborator

@andrjohns andrjohns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks!

@andrjohns andrjohns merged commit 722d196 into master Jun 8, 2024
11 checks passed
@andrjohns andrjohns deleted the fix/issue-975-inits branch June 8, 2024 17:31
@mhollanders
Copy link

@mhollanders would you mind installing the branch of cmdstanr and checking whether this works for the larger model you are working with? I have this passing your minimal test case but would rather be certain I'm not just catching a small edge case

I checked and it works with my model. Thanks!

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.

Dimension mismatch when using Pathfinder object as initial values
3 participants