Timed iteration updates and backward compatibility standards

I think we need to build a separate logger that has a prefix; I did a little digging and didn’t see one that had a prefix in Stan.

Do you want help doing that? It can live in RStan for now and we can move it into Stan later.

Like this?

#include <stan/callbacks/logger.hpp>
#include <ostream>
#include <string>
#include <sstream>

namespace stan {
  namespace callbacks {

    class stream_logger_with_chain_id : stream_logger {
    private:
      const int chain_id_;
    public:
      stream_logger(std::ostream& debug,
                    std::ostream& info,
                    std::ostream& warn,
                    std::ostream& error,
                    std::ostream& fatal, 
                    const int chain_id)
        : debug_(debug), info_(info), warn_(warn), error_(error),
          fatal_(fatal), chain_id_(chain_id) { }

      void debug(const std::string& message) {
        debug_ << "Chain: " << chain_id_ << message << std::endl;
      }

      void debug(const std::stringstream& message) {
        debug_ << "Chain: " << chain_id_ << message.str() << std::endl;
      }

      void info(const std::string& message) {
        info_ << "Chain: " << chain_id_ << message << std::endl;
      }

      void info(const std::stringstream& message) {
        info_ << "Chain: " << chain_id_ << message.str() << std::endl;
      }

      void warn(const std::string& message) {
        warn_ << "Chain: " << chain_id_ << message << std::endl;
      }

      void warn(const std::stringstream& message) {
        warn_ << "Chain: " << chain_id_ << message.str() << std::endl; 
      }

      void error(const std::string& message) {
        error_ << "Chain: " << chain_id_ << message << std::endl;
      }

      void error(const std::stringstream& message) {
        error_ << "Chain: " << chain_id_ << message.str() << std::endl;
      }

      void fatal(const std::string& message) {
        fatal_ << "Chain: " << chain_id_ << message << std::endl;
      }

      void fatal(const std::stringstream& message) {
        fatal_ << "Chain: " << chain_id_ << message.str() << std::endl;
      }
    };

  }
}
#endif

Yes, that’ll work. Although, I think you can just have logger as the parent class instead of stream_logger since you’ve implemented all the functions.

If you do that, I’d suggest writing a simple helper method

void copy_string(const std::string& x, std::ostraem& o) {
  o << x;
}
void copy_string(const std::stringstream& x, std::ostream& o) {
  o << x.str();
}

and then you can template out all the functions to make them more generic.

template <typename T>
void error(const T& msg) {
  error_ << "Chain " << chain_id_  << ": ";  // need a space at least here
  copy_string(msg, error_);
  error_ << std::endl;  // flushes buffer
}

But then the usage is going to be:

logger.fatal(msg);

which is pretty limiting if you need to compose that msg out of pieces.

Another design option that we’ve tossed around would be:

std::ostream& error() { 
  error_ << "Chain: " << chain_id_ << " ";
  return error_;
}

so that the usage can be:

logger.error()
   << "expected y > 0, found y = " << y 
  << "; other messages = " << msg.str()  << std::endl;

That is, you can now output numbers, but you’d have to do your own stream conversions.

I don’t see how to encapsulate the endline, though, which is awkward, as we really want one message per line.

The other option is to have the calls be variadic and have them write out each value. So that’d allow uses like:

logger.error("expected y > 0", "; found y = ", y, "; other messages=", msg);

and then the logger can do all the conversions with to_string() and the end-of-line is easy.

That did not turn out to be a correct assumption. When I use the old stream_logger, it prints the exception message in a reject() statement, but when I use the stream_logger_with_chain_id that inherits directly from logger, it eats the exception messages in a reject() statement. What is driving the different in behavior between

and

This is

I responded to this on the issue. Rather than overriding the definition of the virtual base method, the new definition introduced an overload that let the original method still be called because it was more specific.