Skip to content

Conversation

@andrjohns
Copy link
Collaborator

@andrjohns andrjohns commented Mar 23, 2023

Submission Checklist

  • Run unit tests
  • Declare copyright holder and agree to license (see below)

Summary

This PR addresses #718 and #646 by adding a show_exceptions argument to the sample() and sample_mpi() methods, and also expands the use of show_messages=FALSE to completely suppress all printing of non-errors.

Copyright and Licensing

Please list the copyright holder for the work you are submitting
(this will be you or your assignee, such as a university or company):
Andrew Johnson

By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the following licenses:

@andrjohns
Copy link
Collaborator Author

This PR is technically changing user-facing behaviour, so it should probably get signed off by both @jgabry and @rok-cesnovar.

The current value of show_messages is used for both stdout and stderr handling. This PR changes this to have show_exceptions refer to stderr behaviour and show_messages refer to stdout.

Is that alright with both of you or should we keep it as a single show_messages argument (but with the expansions to suppress more output)?

@codecov-commenter
Copy link

Codecov Report

Merging #746 (5440f47) into master (4c4dd09) will decrease coverage by 0.01%.
The diff coverage is 90.90%.

❗ Current head 5440f47 differs from pull request most recent head d24a359. Consider uploading reports for the commit d24a359 to get more accurate results

@@            Coverage Diff             @@
##           master     #746      +/-   ##
==========================================
- Coverage   86.20%   86.20%   -0.01%     
==========================================
  Files          12       12              
  Lines        4082     4095      +13     
==========================================
+ Hits         3519     3530      +11     
- Misses        563      565       +2     
Impacted Files Coverage Δ
R/run.R 93.73% <88.88%> (-0.21%) ⬇️
R/model.R 90.43% <100.00%> (+0.02%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@andrjohns andrjohns changed the title Update handling of show_messages, add show_errors Update handling of show_messages, add show_exceptions Mar 23, 2023
Copy link
Member

@rok-cesnovar rok-cesnovar left a comment

Choose a reason for hiding this comment

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

I like this. It does change user-facing outputs, but I think the current show_messages was not doing everything it should have been, so most users were using suppress... R functions anyways in cases where show_messages would have been applicable.

Copy link
Member

@jgabry jgabry left a comment

Choose a reason for hiding this comment

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

I like this. It does change user-facing outputs, but I think the current show_messages was not doing everything it should have been, so most users were using suppress... R functions anyways in cases where show_messages would have been applicable.

Agree!

@andrjohns andrjohns merged commit a30d3e9 into master Mar 24, 2023
@rok-cesnovar rok-cesnovar deleted the stdout-and-stderr branch June 26, 2023 10:31
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.

5 participants