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

Issue #337: Use family as string in Ebola vignette #423

Closed
wants to merge 2 commits into from

Conversation

athowes
Copy link
Collaborator

@athowes athowes commented Nov 6, 2024

Description

This is a draft PR to work on #337.

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.

@athowes athowes marked this pull request as draft November 6, 2024 12:36
@athowes
Copy link
Collaborator Author

athowes commented Nov 7, 2024

Some things to try here:

  • Try running ebola.Rmd locally on a Windows machine to see if the error reproduces
  • Ensure any file paths in ebola.Rmd are OK on Windows
  • Use options(stringsAsFactors = FALSE): some R functions (especially older ones) may behave differently on Windows if stringsAsFactors is set differently
  • Check for warnings suppressed on Ubuntu
  • Check for differences in locale or encoding and set the locale explicitly in the R session for the vignette and specify encoding (e.g., UTF-8) in ebola.Rmd to prevent issues.

@athowes athowes linked an issue Nov 7, 2024 that may be closed by this pull request
@athowes
Copy link
Collaborator Author

athowes commented Nov 13, 2024

@seabbs to have a look at this.

@seabbs
Copy link
Contributor

seabbs commented Nov 13, 2024

I don't want to be annoying and I know I drove this but I have just gone through the brms vignettes and help files and it looks like they are moving more and more towards using lognormal() vs a string.

Given that and given our annoying problem here shall we just return to doing so as well?

we dropped this before because we needed to write brms for it to work and it was clunky. When we made the change we also suggested we could export just the families we support (as we need to write custom functionality) and I think this does make sense.

@seabbs
Copy link
Contributor

seabbs commented Nov 13, 2024

We would do this using this approach and enforce all new models to hit all currently supported families: https://forum.posit.co/t/best-practices-for-documenting-re-exports/89254

@seabbs
Copy link
Contributor

seabbs commented Nov 13, 2024

If we agree we want to do this I am happy to make the code changes ahead of the release next week (also happy for @athowes to cover)

@athowes
Copy link
Collaborator Author

athowes commented Nov 13, 2024

I'm fine with using brms::lognormal() if we can't resolve this bug.

@seabbs
Copy link
Contributor

seabbs commented Nov 13, 2024

So I am saying we reexport and so don't use brms:: just lognormal()

@athowes
Copy link
Collaborator Author

athowes commented Nov 14, 2024

Yes I am on the same page. Was just using brms::lognormal() to clarify which function rather than that we explicitly call it (but can see how that would have been confusing).

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.

Use string in Ebola vignette and pass R CMD CHECK on Windows latest
2 participants