metrumresearchgroup / bbr

R interface for model and project management
https://metrumresearchgroup.github.io/bbr/
Other
23 stars 2 forks source link

Add .overwrite argument to submit_model and submit_model #547

Closed kyleam closed 1 year ago

kyleam commented 2 years ago

This extracts two commits from @seth127's bbr_alpha_stan branch that address #544.

At this point, bbr_alpha_stan serves as the base for the bbr.bayes split (#543), so I'd prefer we not actually remove the corresponding commits (902a13d8, 1c44d1c3) from that branch. However, it would be fine to merge this PR rather than wait on #543 to be merged. (That'd just lead to a minor conflict and two sets of commits making these changes.)


I'm marking this as a draft because the plan is to merge this after the upcoming release.

Leftover bits:

Closes #544.

seth127 commented 1 year ago

Looking at https://github.com/metrumresearchgroup/bbr.bayes/pull/30 and the associated changes in https://github.com/metrumresearchgroup/bbr/tree/nmbayes and #543 ... should we prioritize moving this PR along?

I don't fully have my head around all the implications, but:

Any thoughts on that @kyleam ? If we're on the same page, I'd vote to move this to https://github.com/metrumresearchgroup/bbr/milestone/27 (which may end of being 1.5.0, not sure yet...)

kyleam commented 1 year ago
  • I think merging this sooner than later will minimize how tangled things get

It definitely doesn't hurt to get this settled. Conceptually I would want it to land before bbr-bayes-split so that these changes come in through a dedicated PR rather than being brought in as part of the mega bbr-bayes-split changeset.

  • I'm not aware of any reason not to merge this before the next bbr release (aside from the two"leftover bits" mentioned above)

I think the main reason is...

Any thoughts on that @kyleam ? If we're on the same page, I'd vote to move this to https://github.com/metrumresearchgroup/bbr/milestone/27 (which may end of being 1.5.0, not sure yet...)

... this last bit. If we wanted to have a patch-bump bugfix release, then this doesn't belong in it. If we decide we'll go straight for 1.5.0, then this works [*]

Regardless, I'll work on getting this ready to merge so that we can pull the trigger when we decide it's time.

[*] Of course if we keep on hitting into these sorts of issues, we can have two branches so that we can cut patch releases from the bugfix branch, but as we've discussed before, that's probably a bit too complex for the workflow that's actually been used for bbr. (Even back when there were multiple long-lived branches, they weren't used in practice to isolate changes this way. )

seth127 commented 1 year ago

@kyleam are we good to merge this and close https://github.com/metrumresearchgroup/bbr/issues/544 ?

kyleam commented 1 year ago

@kyleam are we good to merge this and close #544 ?

I was going to double check with you tomorrow that you're okay with committing to 1.5.0 for the next release (though I assumed that was the case based on you approving this after I mentioned it and what you said here). So, yes, it's good to merge.