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

181: pre-assign sizes to variables #182

Merged
merged 8 commits into from
Dec 11, 2024
Merged

181: pre-assign sizes to variables #182

merged 8 commits into from
Dec 11, 2024

Conversation

sbfnk
Copy link
Contributor

@sbfnk sbfnk commented Dec 11, 2024

Description

This PR closes #181.

It fixes the issue by assigning sizes to int first (where type conversion is permitted) and then brace-initialise.

If we agree on the solution I'll add a new news item.

Checklist

  • My PR is based on a package issue and I have explicitly linked it.
  • I have included the target issue or issues in the PR title in the for Issue(s) issue-numbers: PR title
  • I have read the contribution guidelines.
  • I have tested my changes locally.
  • I have added or updated unit tests where necessary.
  • I have updated the documentation if required.
  • My code follows the established coding standards.
  • I have added a news item linked to this PR.
  • I have reviewed CI checks for this PR and addressed them as far as I am able.

@sbfnk
Copy link
Contributor Author

sbfnk commented Dec 11, 2024

alternative approach would be to pass these to the function as they're available as data anyway

Copy link
Contributor

@seabbs seabbs left a comment

Choose a reason for hiding this comment

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

Nice. Do you think this makes the code easier or harder to parse? I think for me maybe it is a little harder like this but only a little

There is also some speed considerations (annoyingly touchstone is down) my assumption is that this approach would be slightly faster due to the reduction in repeated work.

I think with the latter considerations probably worth doing.

Also quite a few contributions now so I guess its aut time (wow).

alternative approach would be to pass these to the function as they're available as data anyway

I think we want to avoid this as the more you have to pass the harder these functions become to use for people in my view

@sbfnk
Copy link
Contributor Author

sbfnk commented Dec 11, 2024

Nice. Do you think this makes the code easier or harder to parse? I think for me maybe it is a little harder like this but only a little

I would agree with this, and that it's worth doing overall (especially as it won't work on my laptop with the latest version of cmdstan otherwise).

@sbfnk
Copy link
Contributor Author

sbfnk commented Dec 11, 2024

Also quite a few contributions now so I guess its aut time (wow).

Half a day of contributions - depends on where you want to set the bar really.

@seabbs
Copy link
Contributor

seabbs commented Dec 11, 2024

Half a day of contributions - depends on where you want to set the bar really.

I want to set it very low

@seabbs seabbs enabled auto-merge (squash) December 11, 2024 13:40
seabbs
seabbs previously approved these changes Dec 11, 2024
Copy link
Contributor

@seabbs seabbs left a comment

Choose a reason for hiding this comment

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

Nice LGTM

@seabbs seabbs merged commit 5e06f5c into epinowcast:main Dec 11, 2024
12 of 13 checks passed
@sbfnk sbfnk deleted the fix-181 branch December 11, 2024 14:37
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.

Revert to using size over num_elements in stan code
2 participants