Skip to content

Fix #533 #534

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 7 commits into from
Closed

Fix #533 #534

wants to merge 7 commits into from

Conversation

fweber144
Copy link
Collaborator

Fixes #533 (how I would imagine it). This builds on top of branch fix531 (from #532), so #532 should be merged first.

@fweber144
fix stan-dev#531
ae15d87
@fweber144
argument return_group_terms was unused (always at its default of `T…
c11bac4 
…RUE`) and

for the fix of stan-dev#531, it is easier if separate `return_group_terms` cases do not
exist, so remove that argument here
@fweber144
fix stan-dev#533
beff1ac
@fweber144 fweber144 mentioned this pull request Aug 23, 2025
Closed
@fweber144 fweber144 mentioned this pull request Aug 24, 2025
Closed
@fweber144
Copy link
Collaborator Author

Tagging @aloctavodia.

@aloctavodia
Copy link
Contributor

Could this just be combined with #532, or do you prefer to keep PRs more "atomic" as they handle slightly different issues?

@fweber144
Copy link
Collaborator Author

Could this just be combined with #532, or do you prefer to keep PRs more "atomic" as they handle slightly different issues?

They could be combined. I tend to prefer more atomic PRs, but I'm not sure if I always adhere to that principle myself 😄 In this case, I guess another motivation for different PRs was that I was a bit less sure if this fix here (in #534) is really correct, in the sense that it reflects @AlejandroCatalina's ideas when he implemented split_group_term(). In the meantime, however, I am quite sure that it must have been this way, so if you prefer to have this in #532, I can combine them.

@fweber144
Copy link
Collaborator Author

@aloctavodia what do you think, should I include this in #532?

@aloctavodia
Copy link
Contributor

I think combining is fine in this case.

@fweber144
Copy link
Collaborator Author

Alright; done. Closing this PR now.

@fweber144 fweber144 closed this Sep 10, 2025
@fweber144 fweber144 mentioned this pull request Sep 10, 2025
Merged
@fweber144 fweber144 deleted the split_group_terms_fixed branch September 10, 2025 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Inconsistency in split_group_term() for add_lower_terms = FALSE
2 participants
@fweber144 @aloctavodia