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

Rcpp::message() ? #1145

Closed
krlmlr opened this issue Mar 21, 2021 · 30 comments
Closed

Rcpp::message() ? #1145

krlmlr opened this issue Mar 21, 2021 · 30 comments

Comments

@krlmlr
Copy link
Contributor

krlmlr commented Mar 21, 2021

I wonder if Rcpp::message() should exist. Rcpp::warning() forwards to Rf_warning(); since there is no Rf_message(), the new function would take care of calling base::message() from R.

@eddelbuettel
Copy link
Member

I am not sure I understand what you are writing about.

Are you suggesting we need to add Rcpp::message() and implement it via a call to the R function message() ?

If so, why? What current problem does this solve?

@eddelbuettel
Copy link
Member

eddelbuettel commented Mar 21, 2021

If RSQLite needs a message() you can probably implement a local one, just how you have a local XPtr header as I re-noticed the other day (and is that something we should clean up together) ?

@krlmlr
Copy link
Contributor Author

krlmlr commented Mar 21, 2021

Of course I could roll this myself, I'd rather not. I really think emitting a message from C/C++ code should be easier than the status quo.

The XPtr.h workaround was for an IDE I no longer use.

@eddelbuettel
Copy link
Member

eddelbuettel commented Mar 21, 2021

Ok, now at least I know what we are talking about :) Net additions are easiest; no side effects to existing code. I may have looked into that at some point myself: the (possible) absense of a genuine C API is a spot of bother but we do go via the R API in one or two other places.

So having now established that "yes, we could" the burden is now on you to convince Team Rcpp why we should, and burden ourselves with the test and maintenance work going forward. We're listening.

@krlmlr
Copy link
Contributor Author

krlmlr commented Mar 21, 2021

I really think emitting a message from C/C++ code should be easier than the status quo. There are various ways to solve this:

  1. introduce a C API in R base
  2. solve this in Rcpp and cpp11 via an R call (as proposed here)
  3. implement directly in user libraries

Unless this is solved here first, I'll eventually go with the third option and propose PRs.

@eddelbuettel
Copy link
Member

eddelbuettel commented Mar 21, 2021

I still don''t understand.

  • the "1." we have no control over as we're not R Core
  • the "3." is orthogonal to what we do here and everybody can do as they please in their packages; simply not an issue for us.

So has it taken us five messages to establish that you are proposing a C++ function with static instantiation of an R function you want to call?

And you still have not addressed the question in my previous post: Why would we need this ?

@krlmlr
Copy link
Contributor Author

krlmlr commented Mar 21, 2021

I hear you're hesitant to take on new code, perhaps because that's more of a liability than an asset. If I contribute, I add tests and can respond to questions regarding the new code.

Personally, I think we need this

  • for symmetry: I typed Rcpp::message() without thinking and was genuinely surprised by the CI failure
  • because the alternative is a mess: three or more lines of code that could be a simple call to <whatever_namespace>::message()

Please decide if this is enough motivation, and if it's a good fit for this package. There's no hurry.

(Thanks for the fast responses!)

@eddelbuettel
Copy link
Member

Less fast response now as we have to sleep sometimes too :)

I completely agree on the need for symmetry, I think I wanted it once or twice too.

So how do we do it then? I presume you may have looked into the R code? We could do the verboten thing and hitting a hidden API, or we could do the simple thing via Rcpp::Function (and document that it'll be slower). Thoughts?

@eddelbuettel
Copy link
Member

I had a very very quick look and there is R_ShowMessages() but on first attempt is not yet suppressable:

> Rcpp::cppFunction("void foo(std::string s) { R_ShowMessage(s.c_str()); }")
> foo("The quick brown fox")
The quick brown fox
> suppressMessages(foo("The quick brown fox"))
The quick brown fox
> 

@eddelbuettel
Copy link
Member

And the reason maybe the logic in src/library/base/R/message.R.

So ... dunno: do we need two, an (unconditional) Rcpp::message() and a (suppressable) Rcpp::message2() (or another name...) that goes via Rcpp::Function() to reap these goodies?

@krlmlr
Copy link
Contributor Author

krlmlr commented Mar 21, 2021

We can do un-suppressible messages today via cerr or the equivalent. Should we stick with the approach via Function only?

@eddelbuettel
Copy link
Member

And ignore R_ShowMessage() ? Hm. That one is so cheap to add that I feel like we should (for that very reason of being cheap).

So your motivation, which, even ten or so messages in, you don't seem have made clear is that you want a printing / messaging mechanism that can be silenced via suppressMessages() ? Is that it? In that case a function wrapper it is.

@eddelbuettel
Copy link
Member

Using Rcpp::Rcerr or Rcout does not show the text is a highlighted colour. Using R_ShowMessages does. Picture below (and I have to be a little careful to generalize as I am running colorout here for kicks):

image

@krlmlr
Copy link
Contributor Author

krlmlr commented Mar 21, 2021

Yeah, I'd think that Rcpp::message() should mimic base::message(). I looked at R_ShowMessage(), but messages that can't be suppressed feel terrible no matter how cheap.

Maybe Rcpp::message() and Rcpp::show_message() ?

@kevinushey
Copy link
Contributor

I think we should just call base::message

@eddelbuettel
Copy link
Member

I am also puzzled by what R_ShowMessage() is there for then :-/ Oh well.

Ok, let's wrap base::message as Rcpp::message() then.

@kevinushey
Copy link
Contributor

Looking at the R sources, it looks like R_ShowMessage() exists primarily for applications embedding R; in theory, an application embedding R could provide their own ptr_R_ShowMessage() implementation to decide what to do when R shows a message via R_ShowMessage().

https://github.com/wch/r-source/blob/210076051305661d27393f790cb4a43e63a97715/src/unix/system.c#L74

However, these "message"s are different from what is exposed to users via base::message(); that is, condition objects of class "message" which write to stderr by default.

@eddelbuettel
Copy link
Member

eddelbuettel commented Mar 21, 2021

A truly minimal implementation is here: 8992d83

I am currently rubbing my chin pondering how I would test for suppressMessage() working in a wrapper around (which it does, I am just not quite sure yet how to operationalize the test... Can one capture messages?

(Looks like one can. Nice.)

@Enchufa2
Copy link
Member

You also have tinytest::expect_message.

@eddelbuettel
Copy link
Member

Hah. Mark and I were DMing and that did not come up. Paging the good Dr @markvanderloo ...

@eddelbuettel
Copy link
Member

I guess it affirms that there was a message. Nice. Will add. From the session in which I was noodling:

> expect_message( foo("ABC") )
----- PASSED      : <-->
 call| expect_message(foo("ABC")) 
> 

@Enchufa2
Copy link
Member

And more:

> expect_message(message("asdf"), "ASDF")
----- FAILED[xcpt]: <-->
 call| expect_message(message("asdf"), "ASDF")
 diff| Found 1 message(s) of class 'message', but not matching 'ASDF'. 
 diff| Showing up to three messages:
 diff| Message 1 of class <simpleMessage, message, condition>:
 diff| 'asdf'

@eddelbuettel
Copy link
Member

When did the classed messages appear? Would we have to condition this? I think I am fine with the one-liner I just added, but good thinking...

@Enchufa2
Copy link
Member

Enchufa2 commented Mar 21, 2021

I think that's a 4.0+ feature, but I was just pointing out that expect_message is able to match patterns too, which should also work for R < 4.0.

@eddelbuettel
Copy link
Member

True, true, but I did it "in longhand" by calling capture,output myself. All good, methinks.

@eddelbuettel eddelbuettel mentioned this issue Mar 21, 2021
4 tasks
@krlmlr
Copy link
Contributor Author

krlmlr commented Mar 23, 2021

Thanks for taking this on!

@stla
Copy link

stla commented May 9, 2022

Hi everyone,

Please how to include a C++ variable in the message? I mean I have this code currently:

Rcpp::Rcout << "Number of points removed: " << nremoved << ".\n";

where nremoved is an integer. Should I convert it to a string and then use Rcpp::collapse?

@stla
Copy link

stla commented May 9, 2022

Ah ok, I found std::to_string and one can concatenate the strings with +, I didn't know that.

@eddelbuettel
Copy link
Member

Please don't spray random usage questions here in old threads. We do have

  • a mailing list for using Rcpp
  • answer questions on StackOverflow.

@stla
Copy link

stla commented May 9, 2022

Ok, sorry, I looked for "Rcpp message" on Google and I found this thread first. I thought my question would be useful for someone else who finds this thread. But ok, sorry again.

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

No branches or pull requests

5 participants