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

Add priorsense compatibility #1354

Merged
merged 9 commits into from
Jun 24, 2024
Merged

Conversation

n-kall
Copy link

@n-kall n-kall commented May 13, 2022

As discussed, this PR adds functionality for power-scaling sensitivity analysis via the priorsense package.
This PR is in progress, and needs to be kept up to date with any changes in priorsense before priorsense is on CRAN.

@paul-buerkner
Copy link
Owner

Thank you!

@paul-buerkner
Copy link
Owner

Hey @n-kall I just wanted to ask about the progress of this PR. Is there anything I can/should do at this point?

@paul-buerkner
Copy link
Owner

@n-kall do you need anything do move this PR forward? Not sure what the latest status of this was.

@n-kall
Copy link
Author

n-kall commented May 12, 2023

@paul-buerkner This PR has taken been delayed for a while now, as I was getting the importance weighted moment matching in a neater state. Once the pareto-k diagnostics are merged into posterior (which should be quite soon), iwmm can go to CRAN and be used by priorsense. At that point the priorsense interface will be fixed and I'll update this PR.

@avehtari
Copy link
Contributor

I'm curious about the status of this

@n-kall
Copy link
Author

n-kall commented Feb 28, 2024

We've made quite some progress on a brmsfit method for iwmm at topipa/iwmm brmsfit-method branch. This has been the main hurdle and once done the brms compatibility with priorsense should be pretty much done.

@n-kall n-kall marked this pull request as ready for review June 5, 2024 11:29
@n-kall
Copy link
Author

n-kall commented Jun 5, 2024

@paul-buerkner I've now updated the integration of priorsense based on how it is done for projpred, to finally get this PR ready. I think this is the simplest way to do things, but let me know if you think there is a more sensible way.

Essentially, the only user-facing function is create_priorsense_data.brmsfit but as explained in the docs, this is called automatically from priorsense functions and shouldn't need to be used directly.

I'd like to submit priorsense to CRAN soon, and I think it's best to move the brms-specific stuff from priorsense to brms before then (which is what this PR would do).

This is now ready for review.

@paul-buerkner
Copy link
Owner

Perfect! Will review and merge it this week.

@paul-buerkner
Copy link
Owner

paul-buerkner commented Jun 6, 2024

I have now done some minor cleaning and the PR looks good to me.

Perhaps one thing we shall change is the example model such that it actually shows some prior sensitivity. Otherwise, the model is a bit boring since the powerscaling barely has any effect here. Would you mind changine the example in this direction?

In any case, I can only merge this PR into brms main once priorsense is on CRAN. This is because brms main needs to be CRAN releasable at any time which it is not if it suggests a package currently not on CRAN or another usable repo.

Also, don't forget to update the DESCRIPTION (new version number) and NEWS files

@paul-buerkner
Copy link
Owner

Also, would you mind adding a few tests based on the internally stored brms test models? At least for local runs (and skipped on CRAN).

@n-kall
Copy link
Author

n-kall commented Jun 6, 2024

I think I addressed your comments, thanks! Let me know if there is more to change.
I'll work on getting priorsense on CRAN and let you know when it is and this can be merged.

@paul-buerkner
Copy link
Owner

Looks good! Thank you! As soon as priorsense is on CRAN, I will merge this PR.

@n-kall
Copy link
Author

n-kall commented Jun 24, 2024

Priorsense (1.0.0) is now on CRAN so this can be merged (maybe bump the version number suggested to 1.0.0)

@paul-buerkner
Copy link
Owner

Excellent! Merging now. Thank you for all your work on this!

@paul-buerkner paul-buerkner merged commit 34ac048 into paul-buerkner:master Jun 24, 2024
0 of 5 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.

3 participants