Clean up code: `.str().length()`

[syclik]

Summary:

There are many instances of code that looks like:

if (msg.str().length() > 0)
  logger.info(msg);

We should either:

• have the info() method ignore empty messages in general
• wrap the call in a function

Additional Information:

Originally from this PR comment: https://github.com/stan-dev/stan/pull/2570#discussion_r201151286

Current Version:

v2.17.1

[Ishvina]

Hi! I am in an intro to swe class that is requiring me to work on my first open-source project. I was wondering if this issue is still open and if so could is this beginner-friendly?

[WardBrian]

Hi @Ishvina -

There are indeed still places where this pattern appears in the Stan code, for example: https://github.com/stan-dev/stan/blob/5621ebde258fe9926c6a36160f12e998e028670e/src/stan/services/optimize/lbfgs.hpp#L96-L97

If you'd like to follow the first suggestion and change the logger, the main implementation of the logger can be found here: https://github.com/stan-dev/stan/blob/develop/src/stan/callbacks/stream_logger.hpp

If you know some C++ I think this is a good beginner project, but building Stan itself can be complicated, especially on Windows machines. There are some helpful guides here and here, and if you need human support you can try the #developers channel on our Slack

[AnikaAChowdhury]

Hi @Ishvina! Are you still working on this project? If you are not, could I work on this? I am in the same class as you.

[Ishvina]

@AnikaAChowdhury yea you can work on it!

[mitzimorris]

wecome Ishvana and Anika! happy to help answer your questions! just curious - where are you taking this swe class and for what program?

[AnikaAChowdhury]

It is Intro to Software Engineering class for CS majors at UF.

[AnikaAChowdhury]

Hi @WardBrian! The Slack link does not work. Could you provide me with the link again? Also, could you elaborate a little more on the issue I have to change? Thank You!

[AnikaAChowdhury]

Could anyone explain to me what is the purpose of this issue and what could be achieved by fixing this issue?

[mitzimorris]

this issue is the result of a suggestion made during code review:

This pattern is everywhere, and while it's simple, I'd rather not see this test everywhere. Can we just have the info() mthod ignore empty messages in general? If we really want a blank line, we can pass info(" ") or info("\n") if it doesn't build one in automatically.

If not that, then maybe wrap this all up in a function that does the test so this pattern isn't cut-and-paste dozens of times.

a grep for this shows that it's used in the following files:

src/stan/model/test_gradients.hpp src/stan/variational/advi.hpp src/stan/services/optimize/lbfgs.hpp src/stan/services/optimize/bfgs.hpp src/stan/services/util/initialize.hpp src/stan/services/sample/standalone_gqs.hpp

therefore fixing this issue would:

• make the logger function more robust
• remove lots and lots of identical bits to code

Martin Fowler's "Refactoring" book is a classic and explains why you want do to code cleanup of this sort.

the invite link for Stan slack is: https://join.slack.com/t/mc-stan/shared_invite/enQtMzAyNzg1ODQ5MDczLTc1M2Q1YzM4ZjY5MzRjMGFlNDcyYzRhOGYxNTRlZjRlZjI2YzYxZjYyMDRlNDYzOTY5YzU5MTgzM2JlZjAxNTk

[mitzimorris]

further on, in the code review discussion - https://github.com/stan-dev/stan/pull/2570#discussion_r201151286 - the reviewer had related request:

[future request]

Our loggers should just take these, as in:

logger.info() << "Gradient evaluation took " << ...;

Or we should have a polyadic info function that could look like:

logger.info("Gradient evaluation took ", deltaT, " seconds");

which was turned into issue https://github.com/stan-dev/stan/issues/2579 (without further elaboration)

[AnikaAChowdhury]

Thank You!

[AnikaAChowdhury]

Do I just fix all the instances and then push the changes? Also, could someone point me out how to run makefile?

[WardBrian]

Hi Anika -

This guide to github pull requests might be useful. The basic idea is you can fork this repository to get your own copy, make the changes there, and then request to merge those changes back into this repository.

To run the Makefile you need to have a version of make installed. This depends on what operating system you are using. If you're on mac or linux, this should be pretty easy using something like apt, yum, or brew. On Windows, it is a bit more complicated, and you might want to look into using something like RTools which includes make and some other useful programs for this project.

If you have make installed, it should be as simple as navigating to the checked-out repository and running make in the main folder. We have some wiki pages on make and our tests