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

Brmsfit method #12

Merged
merged 19 commits into from
Mar 25, 2024
Merged

Brmsfit method #12

merged 19 commits into from
Mar 25, 2024

Conversation

n-kall
Copy link
Collaborator

@n-kall n-kall commented Jan 19, 2024

updates to brmsfit method, passes the tests for me.
Might be still some work to do to clean it up

@n-kall n-kall marked this pull request as draft January 19, 2024 10:01
@n-kall n-kall marked this pull request as ready for review January 19, 2024 10:05
Copy link
Owner

@topipa topipa left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning this up. The problem for me with both my branch and yours is that after running moment matching, accessing the brms fit object returned by moment match gives an error

Error in do.call(cbind, attr(x, "sampler_params")) : 
  second argument must be a list

Does this happen to you or not?

@n-kall
Copy link
Collaborator Author

n-kall commented Jan 27, 2024

Ah, yes this happens to me too. Seems to be the print method is failing. The object contains the new fit but must be missing something

@n-kall
Copy link
Collaborator Author

n-kall commented Mar 5, 2024

I now added a function to create dummy sampler_params so the brmsfit object no longer gives an error on print.

The issue is that the sampler_params are stored for each chain, and moment matching merges all chains. Also, as the draws changed, I am not sure that the NUTS parameters are relevant any more anyway. Now in the modified brmsfit, all sampler_params are just numeric(0)

@topipa
Copy link
Owner

topipa commented Mar 25, 2024

I now added a function to create dummy sampler_params so the brmsfit object no longer gives an error on print.

The issue is that the sampler_params are stored for each chain, and moment matching merges all chains. Also, as the draws changed, I am not sure that the NUTS parameters are relevant any more anyway. Now in the modified brmsfit, all sampler_params are just numeric(0)

Oh that was the missing piece. Thank you for taking care of it! The sampler parameters might be useful for something even after moment matching, but we can set them to 0 for now 👍

@topipa topipa merged commit 946b7f6 into topipa:brmsfit-method Mar 25, 2024
topipa added a commit that referenced this pull request Mar 31, 2024
* add more methods

* add basic brmsfit method

* small changes to brmsfit method

* add print function

* small updates

* add return_k = TRUE for pareto smoothing

* address changes to unconstrain_draws in cmdstanr, posterior pareto-k

* use posterior github version in workflow

* fix workflow deps

* change jacobian_adjustment to jacobian (deprecation warning)

* Posterior updates (#7)

* address changes to unconstrain_draws in cmdstanr, posterior pareto-k

* use posterior github version in workflow

* fix workflow deps

* fix deprecation warning (#11)

* add return_k = TRUE for pareto smoothing

* change jacobian_adjustment to jacobian (deprecation warning)

---------

Co-authored-by: n-kall <n-kall@github.com>

---------

Co-authored-by: n-kall <n-kall@github.com>

* updates to constrain/unconstrain methods

* fixes to brmsfit method

* docs update

* fix namespace warnings

* fix namespace issues

* add dummy sampler_params function for brmsfit

* Brmsfit method (#12)

* add return_k = TRUE for pareto smoothing

* address changes to unconstrain_draws in cmdstanr, posterior pareto-k

* use posterior github version in workflow

* fix workflow deps

* change jacobian_adjustment to jacobian (deprecation warning)

* Posterior updates (#7)

* address changes to unconstrain_draws in cmdstanr, posterior pareto-k

* use posterior github version in workflow

* fix workflow deps

* fix deprecation warning (#11)

* add return_k = TRUE for pareto smoothing

* change jacobian_adjustment to jacobian (deprecation warning)

---------

Co-authored-by: n-kall <n-kall@github.com>

---------

Co-authored-by: n-kall <n-kall@github.com>

* updates to constrain/unconstrain methods

* fixes to brmsfit method

* docs update

* fix namespace warnings

* fix namespace issues

* add dummy sampler_params function for brmsfit

---------

Co-authored-by: n-kall <n-kall@github.com>
Co-authored-by: Topi Paananen <24715100+topipa@users.noreply.github.com>

* run styler

* run styler again

* add return statement

* no parallel tests by default

* document

* remove unneeded import

* clean blank lines

* name log weights appropriately

* name log weights

* document

---------

Co-authored-by: n-kall <n-kall@github.com>
Co-authored-by: n-kall <33577035+n-kall@users.noreply.github.com>
n-kall added a commit to n-kall/iwmm that referenced this pull request Apr 10, 2024
* add more methods

* add basic brmsfit method

* small changes to brmsfit method

* add print function

* small updates

* add return_k = TRUE for pareto smoothing

* address changes to unconstrain_draws in cmdstanr, posterior pareto-k

* use posterior github version in workflow

* fix workflow deps

* change jacobian_adjustment to jacobian (deprecation warning)

* Posterior updates (#7)

* address changes to unconstrain_draws in cmdstanr, posterior pareto-k

* use posterior github version in workflow

* fix workflow deps

* fix deprecation warning (topipa#11)

* add return_k = TRUE for pareto smoothing

* change jacobian_adjustment to jacobian (deprecation warning)

---------



---------



* updates to constrain/unconstrain methods

* fixes to brmsfit method

* docs update

* fix namespace warnings

* fix namespace issues

* add dummy sampler_params function for brmsfit

* Brmsfit method (topipa#12)

* add return_k = TRUE for pareto smoothing

* address changes to unconstrain_draws in cmdstanr, posterior pareto-k

* use posterior github version in workflow

* fix workflow deps

* change jacobian_adjustment to jacobian (deprecation warning)

* Posterior updates (#7)

* address changes to unconstrain_draws in cmdstanr, posterior pareto-k

* use posterior github version in workflow

* fix workflow deps

* fix deprecation warning (topipa#11)

* add return_k = TRUE for pareto smoothing

* change jacobian_adjustment to jacobian (deprecation warning)

---------



---------



* updates to constrain/unconstrain methods

* fixes to brmsfit method

* docs update

* fix namespace warnings

* fix namespace issues

* add dummy sampler_params function for brmsfit

---------




* run styler

* run styler again

* add return statement

* no parallel tests by default

* document

* remove unneeded import

* clean blank lines

* name log weights appropriately

* name log weights

* document

---------

Co-authored-by: Topi Paananen <24715100+topipa@users.noreply.github.com>
Co-authored-by: n-kall <n-kall@github.com>
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