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

[feature-request] Add option file_refit from brms #228

Closed
GidonFrischkorn opened this issue Aug 14, 2024 · 4 comments · Fixed by #234
Closed

[feature-request] Add option file_refit from brms #228

GidonFrischkorn opened this issue Aug 14, 2024 · 4 comments · Fixed by #234
Assignees
Labels
enhancement - new feature New user or developer feature
Milestone

Comments

@GidonFrischkorn
Copy link
Collaborator

We have already implemented the file argument for the bmm function and added some code to pass the relevant information to the brms call. Can we add similar code to adequately pass the file_refit option to brms and check if something has changed in the call to brm when passing the information compiled by the bmm function.

Currently, there is an error message, when you do not use the default option for file_refit (which is never). So calling bmm with file_refit "on_change" returns the error: invalid 'y' type in 'x || y'

@venpopov
Copy link
Owner

venpopov commented Aug 14, 2024

We do not pass the file argument to brms. This is because brms will save the brmsfit object, which we do not want. Instead, we completely reimplemented the functionality to read and save from file.

The file_refit option is also implemented, but only as a boolean - FALSE (default) and TRUE. This is why "on_change" doesn't work. In brms you have 'never', 'on_change', and 'always'. At the time I implemented it that way, because I use external tools to determine if something in a pipeline has changed and whether to rerun some code or not, so I didn't want to bother reimplementing the on_change detection.

This will be difficult to do.brms uses a function brms_needs_refit to determine if the standata or stancode have changed when the argument is set to "on_change". This function is exported, so in principle we could call it. But we will need to generate and pass the stancode and standata to it, which is a lot of extra work and which brms will duplicate internally.

We cannot just pass the argument to brms and let it handle it, because we need to save/load the bmmfit object, not the brmsfit object.

@GidonFrischkorn
Copy link
Collaborator Author

GidonFrischkorn commented Aug 14, 2024

I see. To streamline the UX for users who are familiar with brms, we could allow to pass "never", "always" and throw a warning when someone passes "on_change" and say that this functionality is currently not implemented for bmmodels.

I agree that this is not a high priority, so we can see if at some point we feel that this is a nice extra feature. Or I ask @chenyu-psy, who pointed me towards this issue if he wants to have a look at this. And if yes, he can look into how brms handles this and what it would take so that bmm could also provide this option.

@venpopov
Copy link
Owner

That sounds reasonable

@chenyu-psy
Copy link

Hi, I don't have a strong reason to use the file_refit argument. I noticed this issue because I copied all the settings from the brm function and adjusted them for the bmm. Additionally, file_refit = "on_change" is a default setting in all my brms models. That's why I noticed this issue. I think it would be good enough if there were a warning message showing me how to adjust the code, and I also don't necessarily need it.

@GidonFrischkorn GidonFrischkorn linked a pull request Aug 23, 2024 that will close this issue
@GidonFrischkorn GidonFrischkorn self-assigned this Aug 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement - new feature New user or developer feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants