Skip to content

Comments

Fix bug in IS method error message + documentation typos#326

Open
ishaan-arora-1 wants to merge 1 commit intostan-dev:masterfrom
ishaan-arora-1:fix/typos-and-error-message-bug
Open

Fix bug in IS method error message + documentation typos#326
ishaan-arora-1 wants to merge 1 commit intostan-dev:masterfrom
ishaan-arora-1:fix/typos-and-error-message-bug

Conversation

@ishaan-arora-1
Copy link

Hey! I was going through the codebase and spotted a small bug along with some typos. Figured I'd clean them up in one go.

Bug fix

In R/importance_sampling.R, the function assert_importance_sampling_method_is_implemented() constructs an error message when an unsupported importance sampling method is passed. On line 117, implemented_is_methods is used without ():

paste0(implemented_is_methods, collapse = "', '")

Since implemented_is_methods is a function, this ends up pasting the function object itself into the error message instead of the actual method names ("psis", "tis", "sis"). The condition check on line 113 correctly calls implemented_is_methods(), so this was just missed in the message string.

Typos

While reading through the source I also noticed these scattered across the roxygen comments:

  • diagnostics.R: "dependend" → "dependent", "k-ht" → "k-hat"
  • loo-glossary.R: "The pages provides definitions to" → "The page provides definitions of"
  • split_moment_matching.R: "log-likeliood" → "log-likelihood"
  • loo_subsample.R: "subampled" → "subsampled"
  • loo_compare.psis_loo_ss_list.R: "comaprison" → "comparison"
  • loo_model_weights.R: "matrices or , one for each" → "matrices, one for each"
  • loo-package.R: "widely application" → "widely applicable" (Watanabe 2010 reference)

I also updated the corresponding .Rd man pages to match.

Fixes #325

Fix missing parentheses on `implemented_is_methods` in the error
message inside `assert_importance_sampling_method_is_implemented()`,
which caused the function object to be pasted instead of the actual
method names.

Also fix several typos across roxygen comments and the corresponding
generated .Rd man pages:
- "dependend" -> "dependent" (diagnostics.R)
- "k-ht" -> "k-hat" (diagnostics.R)
- "The pages provides definitions to" -> "The page provides definitions of" (loo-glossary.R)
- "log-likeliood" -> "log-likelihood" (split_moment_matching.R)
- "subampled" -> "subsampled" (loo_subsample.R)
- "comaprison" -> "comparison" (loo_compare.psis_loo_ss_list.R)
- "matrices or , one" -> "matrices, one" (loo_model_weights.R)
- "widely application" -> "widely applicable" (loo-package.R)

Fixes stan-dev#325
@codecov-commenter
Copy link

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 92.78%. Comparing base (2883f25) to head (843ec66).

Files with missing lines Patch % Lines
R/importance_sampling.R 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master     #326   +/-   ##
=======================================
  Coverage   92.78%   92.78%           
=======================================
  Files          31       31           
  Lines        2992     2992           
=======================================
  Hits         2776     2776           
  Misses        216      216           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

Bug in error message for invalid IS method + several documentation typos

2 participants