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

Clean up code: .str().length() #2577

Open
syclik opened this issue Jul 10, 2018 · 13 comments · May be fixed by #3098
Open

Clean up code: .str().length() #2577

syclik opened this issue Jul 10, 2018 · 13 comments · May be fixed by #3098

Comments

@syclik
Copy link
Member

syclik commented Jul 10, 2018

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: #2570 (comment)

Current Version:

v2.17.1

@Ishvina
Copy link

Ishvina commented Jan 20, 2022

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
Copy link
Member

Hi @Ishvina -

There are indeed still places where this pattern appears in the Stan code, for example:

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

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
Copy link

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
Copy link

Ishvina commented Jan 21, 2022

@AnikaAChowdhury yea you can work on it!

@mitzimorris
Copy link
Member

mitzimorris commented Jan 21, 2022

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

@AnikaAChowdhury
Copy link

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

@AnikaAChowdhury
Copy link

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
Copy link

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

@mitzimorris
Copy link
Member

mitzimorris commented Jan 22, 2022

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
Copy link
Member

further on, in the code review discussion - #2570 (comment) -
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 #2579
(without further elaboration)

@AnikaAChowdhury
Copy link

Thank You!

@AnikaAChowdhury
Copy link

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

@WardBrian
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment