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

Automated Style - Fixes #661 #662

Merged
merged 3 commits into from Dec 16, 2017

Conversation

Projects
None yet
5 participants
@seantalts
Member

seantalts commented Oct 27, 2017

Submission Checklist

  • Run unit tests: ./runTests.py test/unit
  • Run cpplint: make cpplint
  • Declare copyright holder and open-source license: see below

Setup

First, install clang-format

Editor setup

Emacs - there's a plugin/script called google-c-style. You can find it here or just install from MELPA.
Spacemacs - you can add google-c-style to dotspacemacs-additional-packages.
Vim- check out https://github.com/google/vim-codefmt

Git hook

There is a pre-commit hook in hooks/pre-commit, and if you run bash hooks/install_hooks.sh it will be installed for you. Going forward, it will run clang-format -i (which implicitly uses the .clang-format file in the repo) on any changed files to format them. If you need to disable this for some reason (though your tests will fail if you haven't formatted things correctly) you can use git commit --no-verify.

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): Columbia University

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

@seantalts

This comment has been minimized.

Show comment
Hide comment
@seantalts

seantalts Oct 27, 2017

Member

Oh man, apparently the include order sorting clang-format does according to the google style guide breaks test-headers (at least). I need to dig into that...

Member

seantalts commented Oct 27, 2017

Oh man, apparently the include order sorting clang-format does according to the google style guide breaks test-headers (at least). I need to dig into that...

@seantalts seantalts changed the title from Automated Style - Fixes #661 to [WIP] Automated Style - Fixes #661 Oct 27, 2017

@bgoodri

This comment has been minimized.

Show comment
Hide comment
@bgoodri

bgoodri Oct 27, 2017

Contributor
Contributor

bgoodri commented Oct 27, 2017

@seantalts

This comment has been minimized.

Show comment
Hide comment
@seantalts

seantalts Oct 27, 2017

Member

In file included from ./stan/math/prim/mat/fun/Eigen.hpp:4:0,
from ./stan/math/rev/mat/functor/gradient.hpp:4,
from http://d1m1s1b1.stat.columbia.edu:8080/job/Math%20Pull%20Request%20-%20Tests%20-%20Header/840/consoleFull

And more, seemingly mostly around our Eigen type aliases?

Member

seantalts commented Oct 27, 2017

In file included from ./stan/math/prim/mat/fun/Eigen.hpp:4:0,
from ./stan/math/rev/mat/functor/gradient.hpp:4,
from http://d1m1s1b1.stat.columbia.edu:8080/job/Math%20Pull%20Request%20-%20Tests%20-%20Header/840/consoleFull

And more, seemingly mostly around our Eigen type aliases?

@bgoodri

This comment has been minimized.

Show comment
Hide comment
@bgoodri

bgoodri Oct 27, 2017

Contributor

It looks like it didn't even change the order of the headers in that file, but rather in stan/math/rev/core.hpp.

Contributor

bgoodri commented Oct 27, 2017

It looks like it didn't even change the order of the headers in that file, but rather in stan/math/rev/core.hpp.

@bgoodri

This comment has been minimized.

Show comment
Hide comment
@bgoodri

bgoodri Oct 27, 2017

Contributor

It was just one line in each of two files. We may have to blacklist some files from clang-format but in general, we should try to make Stan Math not tied to the order of includes and to not be over-including headers.

Contributor

bgoodri commented Oct 27, 2017

It was just one line in each of two files. We may have to blacklist some files from clang-format but in general, we should try to make Stan Math not tied to the order of includes and to not be over-including headers.

@bob-carpenter

This comment has been minimized.

Show comment
Hide comment
@bob-carpenter

bob-carpenter Oct 28, 2017

Contributor
Contributor

bob-carpenter commented Oct 28, 2017

@seantalts

This comment has been minimized.

Show comment
Hide comment
@seantalts

seantalts Oct 28, 2017

Member

I had no idea there were functional issues with the formatting part of Google's style guide that would prevent us from adopting it. I wish those would have come up during the meeting when we talked about this idea a few weeks ago. I'd love to see that list.

I'd also like to learn more about why our includes are so fragile and don't seem to follow what I read about online. Any resource links or other explanation would be appreciated!

Member

seantalts commented Oct 28, 2017

I had no idea there were functional issues with the formatting part of Google's style guide that would prevent us from adopting it. I wish those would have come up during the meeting when we talked about this idea a few weeks ago. I'd love to see that list.

I'd also like to learn more about why our includes are so fragile and don't seem to follow what I read about online. Any resource links or other explanation would be appreciated!

@sakrejda

This comment has been minimized.

Show comment
Hide comment
@sakrejda

sakrejda Oct 28, 2017

Contributor

I'd love to see the list too. I thought include order issues just come from people including file A and B (that depends on A) when B should suffice because B should be including A to begin with. It's boring and time-consuming to resolve that but I didn't think it would require any clever solutions. I saw some circular stuff in the language library that seemed harder to resolve but not in math.

Contributor

sakrejda commented Oct 28, 2017

I'd love to see the list too. I thought include order issues just come from people including file A and B (that depends on A) when B should suffice because B should be including A to begin with. It's boring and time-consuming to resolve that but I didn't think it would require any clever solutions. I saw some circular stuff in the language library that seemed harder to resolve but not in math.

@bob-carpenter

This comment has been minimized.

Show comment
Hide comment
@bob-carpenter

bob-carpenter Oct 29, 2017

Contributor

What do you mean by circular dependencies (in stan-dev/stan in the lang directory, I take it)?

The parser is actually the most independently factored bit of our code because I had to do that to get it to compile on Windows.

The main issue we have with includes is that you need have to complete types before you try to use things and you need to include the types with which templates will get instantiated before the templates.

Our advice is to use math.hpp or one of the other top-level includes, which take care of all of the dependencies.

We might be able to resolve these---I've never been able to do it.

Contributor

bob-carpenter commented Oct 29, 2017

What do you mean by circular dependencies (in stan-dev/stan in the lang directory, I take it)?

The parser is actually the most independently factored bit of our code because I had to do that to get it to compile on Windows.

The main issue we have with includes is that you need have to complete types before you try to use things and you need to include the types with which templates will get instantiated before the templates.

Our advice is to use math.hpp or one of the other top-level includes, which take care of all of the dependencies.

We might be able to resolve these---I've never been able to do it.

@bob-carpenter

This comment has been minimized.

Show comment
Hide comment
@bob-carpenter

bob-carpenter Oct 29, 2017

Contributor
Contributor

bob-carpenter commented Oct 29, 2017

@syclik

This comment has been minimized.

Show comment
Hide comment
@syclik

syclik Oct 29, 2017

Member

@seantalts, hopefully you've seen this wiki doc that lists most of our deviations from Google Style: https://github.com/stan-dev/stan/wiki/Coding-Style-and-Idioms

The include order thing is a biggie. I can respond here, but anyone mind if I start it on Discourse instead? Discussing it over a pull request is ok, but it'll essentially be lost / hard to search once we close the pull request.

Member

syclik commented Oct 29, 2017

@seantalts, hopefully you've seen this wiki doc that lists most of our deviations from Google Style: https://github.com/stan-dev/stan/wiki/Coding-Style-and-Idioms

The include order thing is a biggie. I can respond here, but anyone mind if I start it on Discourse instead? Discussing it over a pull request is ok, but it'll essentially be lost / hard to search once we close the pull request.

@seantalts

This comment has been minimized.

Show comment
Hide comment
@seantalts

seantalts Oct 29, 2017

Member

Yep, I've seen that doc. The include order thing probably warrants a discourse thread, this is true. I would be absolutely thrilled to get rid of the 12 way factorization we have going on - this has been a huge source of my own struggle in onboarding and continuing maintenance / development.

Member

seantalts commented Oct 29, 2017

Yep, I've seen that doc. The include order thing probably warrants a discourse thread, this is true. I would be absolutely thrilled to get rid of the 12 way factorization we have going on - this has been a huge source of my own struggle in onboarding and continuing maintenance / development.

@syclik

This comment has been minimized.

Show comment
Hide comment
@syclik

syclik Oct 29, 2017

Member
Member

syclik commented Oct 29, 2017

@seantalts

This comment has been minimized.

Show comment
Hide comment
@seantalts

seantalts Oct 29, 2017

Member

The thing Bob is talking about above, prim/rev/fwd/mix * scal/arr/mat. He called it a "12-way distinction" above.

Member

seantalts commented Oct 29, 2017

The thing Bob is talking about above, prim/rev/fwd/mix * scal/arr/mat. He called it a "12-way distinction" above.

@seantalts

This comment has been minimized.

Show comment
Hide comment
@seantalts

seantalts Oct 29, 2017

Member

This now has the changes we talked about on the issue. https://github.com/stan-dev/math/pull/662/files?w=1 to see non-whitespace changes.

Member

seantalts commented Oct 29, 2017

This now has the changes we talked about on the issue. https://github.com/stan-dev/math/pull/662/files?w=1 to see non-whitespace changes.

@seantalts seantalts changed the title from [WIP] Automated Style - Fixes #661 to Automated Style - Fixes #661 Oct 29, 2017

@seantalts

This comment has been minimized.

Show comment
Hide comment
@seantalts

seantalts Oct 31, 2017

Member

Anyone want to review this? I assume the review would mostly be checking out the pre-commit hook, install script, verifying my instructions, and verifying the steps I used to format the world:

find stan test -name '*.hpp' -o -name '*.cpp' | xargs clang-format -i
Member

seantalts commented Oct 31, 2017

Anyone want to review this? I assume the review would mostly be checking out the pre-commit hook, install script, verifying my instructions, and verifying the steps I used to format the world:

find stan test -name '*.hpp' -o -name '*.cpp' | xargs clang-format -i
@bob-carpenter

This comment has been minimized.

Show comment
Hide comment
@bob-carpenter

bob-carpenter Oct 31, 2017

Contributor

Is there a written guide somewhere to what code's supposed to look like? From what I've reviewed, I can live with it. It'd be best if @syclik could review the continuous integration bit.

Contributor

bob-carpenter commented Oct 31, 2017

Is there a written guide somewhere to what code's supposed to look like? From what I've reviewed, I can live with it. It'd be best if @syclik could review the continuous integration bit.

@seantalts

This comment has been minimized.

Show comment
Hide comment
@seantalts

seantalts Oct 31, 2017

Member

A written guide? I think clang-format is trying to follow the Google C++ style guide for formatting as much as possible. You can see what it's capable of here: https://clang.llvm.org/docs/ClangFormatStyleOptions.html

And you can see the settings of the options in the PR in the .clang-format file, linked:
https://github.com/stan-dev/math/blob/694a478e8eb2ccddf102552233b229cb4e95224b/.clang-format

Member

seantalts commented Oct 31, 2017

A written guide? I think clang-format is trying to follow the Google C++ style guide for formatting as much as possible. You can see what it's capable of here: https://clang.llvm.org/docs/ClangFormatStyleOptions.html

And you can see the settings of the options in the PR in the .clang-format file, linked:
https://github.com/stan-dev/math/blob/694a478e8eb2ccddf102552233b229cb4e95224b/.clang-format

@bob-carpenter

This comment has been minimized.

Show comment
Hide comment
@bob-carpenter

bob-carpenter Oct 31, 2017

Contributor

Overall, this looks great. The stuff I care about (with one exception) is done automatically here.

Is the idea it just runs when people submit code so that when the code gets merged it is processed? I'm OK with that.

The one change I'd like is:

BreakBeforeBinaryOperators: BOS_All

I don't like that it breaks after "(" in functions rather than moving the function name down, but I can live with it. I didn't see a simple control for it.

Contributor

bob-carpenter commented Oct 31, 2017

Overall, this looks great. The stuff I care about (with one exception) is done automatically here.

Is the idea it just runs when people submit code so that when the code gets merged it is processed? I'm OK with that.

The one change I'd like is:

BreakBeforeBinaryOperators: BOS_All

I don't like that it breaks after "(" in functions rather than moving the function name down, but I can live with it. I didn't see a simple control for it.

@syclik

This comment has been minimized.

Show comment
Hide comment
@syclik

syclik Nov 1, 2017

Member

I'll take a look at the continuous integration bit.

A style question:
Can we change the AllowShortIfStatementsOnASingleLine and AllowShortLoopsOnASingleLine to false? I'm having trouble reading and keeping track of the conditionals and loops when they're all on the same line. The Google Style Guide on conditionals says "short conditional statements may be written on one line if this enhances readability." But it looks like clang-format is forcing all short conditionals to be on one line instead of it sticking to what's been written in the code. If everyone else feels the opposite about this, I can live with it.

Member

syclik commented Nov 1, 2017

I'll take a look at the continuous integration bit.

A style question:
Can we change the AllowShortIfStatementsOnASingleLine and AllowShortLoopsOnASingleLine to false? I'm having trouble reading and keeping track of the conditionals and loops when they're all on the same line. The Google Style Guide on conditionals says "short conditional statements may be written on one line if this enhances readability." But it looks like clang-format is forcing all short conditionals to be on one line instead of it sticking to what's been written in the code. If everyone else feels the opposite about this, I can live with it.

@syclik

This comment has been minimized.

Show comment
Hide comment
@syclik

syclik Nov 1, 2017

Member

I'll start with a simpler question: how do I install clang-format? Did you use a package manager for it?

Member

syclik commented Nov 1, 2017

I'll start with a simpler question: how do I install clang-format? Did you use a package manager for it?

@syclik

This comment has been minimized.

Show comment
Hide comment
@syclik

syclik Nov 1, 2017

Member

wait... I just found out the original post had a link embedded in it. (I didn't notice it the first time.) I'll follow those instructions.

Member

syclik commented Nov 1, 2017

wait... I just found out the original post had a link embedded in it. (I didn't notice it the first time.) I'll follow those instructions.

@syclik

This comment has been minimized.

Show comment
Hide comment
@syclik

syclik Nov 1, 2017

Member

I verified that it works as a pre-commit hook, but had a few questions:

  • could we indicate which files were automatically formatted instead of the files that were considered for change?
  • can you try to install clang-format on the Windows box?
  • I find it a bit odd that the user commit isn't recoverable once clang-format works on the files. Is that how people use these tools? Is that what you intended? (The alternative is to commit using whatever style was committed, then generating the change as a separate change -- possibly not even a separate commit.)
Member

syclik commented Nov 1, 2017

I verified that it works as a pre-commit hook, but had a few questions:

  • could we indicate which files were automatically formatted instead of the files that were considered for change?
  • can you try to install clang-format on the Windows box?
  • I find it a bit odd that the user commit isn't recoverable once clang-format works on the files. Is that how people use these tools? Is that what you intended? (The alternative is to commit using whatever style was committed, then generating the change as a separate change -- possibly not even a separate commit.)
@bob-carpenter

This comment has been minimized.

Show comment
Hide comment
@bob-carpenter

bob-carpenter Nov 1, 2017

Contributor
Contributor

bob-carpenter commented Nov 1, 2017

@seantalts

This comment has been minimized.

Show comment
Hide comment
@seantalts

seantalts Nov 1, 2017

Member

Jenkins, retest this please.

Member

seantalts commented Nov 1, 2017

Jenkins, retest this please.

1 similar comment
@seantalts

This comment has been minimized.

Show comment
Hide comment
@seantalts

seantalts Nov 2, 2017

Member

Jenkins, retest this please.

Member

seantalts commented Nov 2, 2017

Jenkins, retest this please.

@seantalts

This comment has been minimized.

Show comment
Hide comment
@seantalts

seantalts Nov 2, 2017

Member

I'm not sure how to reply to stuff on github. But to reply to @syclik:

could we indicate which files were automatically formatted instead of the files that were considered for change?

I could do that, sure.

can you try to install clang-format on the Windows box?

Why?

I find it a bit odd that the user commit isn't recoverable once clang-format works on the files. Is that how people use these tools? Is that what you intended? (The alternative is to commit using whatever style was committed, then generating the change as a separate change -- possibly not even a separate commit.)

This is how people normally use these tools, yes.

Member

seantalts commented Nov 2, 2017

I'm not sure how to reply to stuff on github. But to reply to @syclik:

could we indicate which files were automatically formatted instead of the files that were considered for change?

I could do that, sure.

can you try to install clang-format on the Windows box?

Why?

I find it a bit odd that the user commit isn't recoverable once clang-format works on the files. Is that how people use these tools? Is that what you intended? (The alternative is to commit using whatever style was committed, then generating the change as a separate change -- possibly not even a separate commit.)

This is how people normally use these tools, yes.

@bob-carpenter

This comment has been minimized.

Show comment
Hide comment
@bob-carpenter

bob-carpenter Nov 2, 2017

Contributor
Contributor

bob-carpenter commented Nov 2, 2017

@syclik

This comment has been minimized.

Show comment
Hide comment
@syclik

syclik Nov 7, 2017

Member

Please don't do that to prove a point. Both those changes seem like good ones.

Member

syclik commented Nov 7, 2017

Please don't do that to prove a point. Both those changes seem like good ones.

@seantalts

This comment has been minimized.

Show comment
Hide comment
@seantalts

seantalts Nov 7, 2017

Member

Everyone agreed to go with full automated Google in order to prevent these kinds of conversations, of which I've had a few already. You're right that having any exceptions will cause them to crop up. I am doing this for the harmony of the team.

Member

seantalts commented Nov 7, 2017

Everyone agreed to go with full automated Google in order to prevent these kinds of conversations, of which I've had a few already. You're right that having any exceptions will cause them to crop up. I am doing this for the harmony of the team.

@syclik

This comment has been minimized.

Show comment
Hide comment
@syclik

syclik Nov 7, 2017

Member
Member

syclik commented Nov 7, 2017

@seantalts

This comment has been minimized.

Show comment
Hide comment
@seantalts

seantalts Nov 7, 2017

Member

After talking it over in the office, I think it would be okay to compromise on this and the operators thing since they are allowed either way in the manual, and provided we never change the .clang-format file again. Re: for and if statements, both @bob-carpenter and @bgoodri want it on one line if it fits (current behavior). You want it always on two. I slightly prefer one line. Are you okay with going with the rest of us on this or do you want to ask more developers?

Member

seantalts commented Nov 7, 2017

After talking it over in the office, I think it would be okay to compromise on this and the operators thing since they are allowed either way in the manual, and provided we never change the .clang-format file again. Re: for and if statements, both @bob-carpenter and @bgoodri want it on one line if it fits (current behavior). You want it always on two. I slightly prefer one line. Are you okay with going with the rest of us on this or do you want to ask more developers?

@sakrejda

This comment has been minimized.

Show comment
Hide comment
@sakrejda

sakrejda Nov 7, 2017

Contributor
Contributor

sakrejda commented Nov 7, 2017

@bob-carpenter

This comment has been minimized.

Show comment
Hide comment
@bob-carpenter

bob-carpenter Nov 7, 2017

Contributor
Contributor

bob-carpenter commented Nov 7, 2017

@sakrejda

This comment has been minimized.

Show comment
Hide comment
@sakrejda

sakrejda Nov 7, 2017

Contributor
Contributor

sakrejda commented Nov 7, 2017

@bob-carpenter

This comment has been minimized.

Show comment
Hide comment
@bob-carpenter

bob-carpenter Nov 7, 2017

Contributor
Contributor

bob-carpenter commented Nov 7, 2017

@seantalts

This comment has been minimized.

Show comment
Hide comment
@seantalts

seantalts Nov 7, 2017

Member

I actually reverted the include order changes because they weren't passing tests. We can't change include order with the tool right now (maybe ever) due to our special include situation (forget the exact details but a few things need to be in a specific order that isn't alphabetical).

We can say PRs have to format with the autoformatter. I'll copy the instructions up top into the wiki (and maybe add something to the pull request template). We can still reference the Google style guide and have our wiki of exceptions (which I also need to update when this goes in) - fewer now I hope.

Member

seantalts commented Nov 7, 2017

I actually reverted the include order changes because they weren't passing tests. We can't change include order with the tool right now (maybe ever) due to our special include situation (forget the exact details but a few things need to be in a specific order that isn't alphabetical).

We can say PRs have to format with the autoformatter. I'll copy the instructions up top into the wiki (and maybe add something to the pull request template). We can still reference the Google style guide and have our wiki of exceptions (which I also need to update when this goes in) - fewer now I hope.

@bob-carpenter

This comment has been minimized.

Show comment
Hide comment
@bob-carpenter

bob-carpenter Nov 8, 2017

Contributor
Contributor

bob-carpenter commented Nov 8, 2017

@seantalts

This comment has been minimized.

Show comment
Hide comment
@seantalts

seantalts Nov 8, 2017

Member

Speaking of the meeting, I really don't want to bring it up at the meeting unless it's to take a quick poll or something. Need a quick decision criteria; no one is going to be "convinced" on this issue either way and there's no hope for consensus.

Member

seantalts commented Nov 8, 2017

Speaking of the meeting, I really don't want to bring it up at the meeting unless it's to take a quick poll or something. Need a quick decision criteria; no one is going to be "convinced" on this issue either way and there's no hope for consensus.

@seantalts

This comment has been minimized.

Show comment
Hide comment
@seantalts

seantalts Nov 8, 2017

Member

I am calling things automated by this PR "formatting" (or maybe even more specifically, automatable formatting) and hoping to eliminate things that are in that domain from our discussions for good. Whatever other rules The Code Tsars or Committers or whoever want are a separate discussion. That's everything else - references vs pointers, exceptions, modularity, code complexity, etc.. Would eventually also like to get a reasonable static analysis tool (clang-tidy seems like a decent candidate) running on our codebase, but that's also separate. I want to make PRs easy to review and easy to submit.

Member

seantalts commented Nov 8, 2017

I am calling things automated by this PR "formatting" (or maybe even more specifically, automatable formatting) and hoping to eliminate things that are in that domain from our discussions for good. Whatever other rules The Code Tsars or Committers or whoever want are a separate discussion. That's everything else - references vs pointers, exceptions, modularity, code complexity, etc.. Would eventually also like to get a reasonable static analysis tool (clang-tidy seems like a decent candidate) running on our codebase, but that's also separate. I want to make PRs easy to review and easy to submit.

@syclik

This comment has been minimized.

Show comment
Hide comment
@syclik

syclik Nov 8, 2017

Member

I gave it more thought. Something to address up front: I really hope we don't treat process/tooling as something that's frozen forever. We own it. We should be able to change it to help us out as we move forward. That said, I don't want to revisit this again for a long time.

On to the two flags I'm suggesting we change: I'm still requesting they be turned to false. I don't think I clearly articulated the issue. It seems like Krzysztof and myself find this to be a readability issue, not a cosmetic issue. I think this is asymmetric, but please let me know if I'm mistaken. Bob, Ben, or Sean, if conditionals and loops are on two lines, do you find it hard to read or hard to understand the code? For whatever reason, I find it difficult to follow the conditionals and loops when they're on the same line. I'm sure I can learn to handle it the other way, but if it's just a matter of aesthetics, then I think it's better for the team overall if we help readability. (If it were the other way around where someone found it to be hard to comprehend one way and my reason for wanting another was cosmetic, I'd gladly go along the other way for the team.)

Here's a concrete example that, for whatever reason, I find easier to read with it the first way instead of the second. From chi_square_cdf.hpp:

The way I'm requesting:

template <typename T_y, typename T_dof>
typename return_type<T_y, T_dof>::type chi_square_cdf(const T_y& y,
                                                      const T_dof& nu) {
  static const std::string function = "chi_square_cdf";
  typedef
      typename stan::partials_return_type<T_y, T_dof>::type T_partials_return;

  T_partials_return cdf(1.0);

  if (!(stan::length(y) && stan::length(nu)))
    return cdf;

  check_not_nan(function, "Random variable", y);
  check_nonnegative(function, "Random variable", y);
  check_positive_finite(function, "Degrees of freedom parameter", nu);
  check_consistent_sizes(function, "Random variable", y,
                         "Degrees of freedom parameter", nu);

  scalar_seq_view<T_y> y_vec(y);
  scalar_seq_view<T_dof> nu_vec(nu);
  size_t N = max_size(y, nu);

  operands_and_partials<T_y, T_dof> ops_partials(y, nu);

  // Explicit return for extreme values
  // The gradients are technically ill-defined, but treated as zero
  for (size_t i = 0; i < stan::length(y); i++) {
    if (value_of(y_vec[i]) == 0)
      return ops_partials.build(0.0);
  }

The way it is in this PR:

template <typename T_y, typename T_dof>
typename return_type<T_y, T_dof>::type chi_square_cdf(const T_y& y,
                                                      const T_dof& nu) {
  static const std::string function = "chi_square_cdf";
  typedef
      typename stan::partials_return_type<T_y, T_dof>::type T_partials_return;

  T_partials_return cdf(1.0);

  if (!(stan::length(y) && stan::length(nu))) return cdf;

  check_not_nan(function, "Random variable", y);
  check_nonnegative(function, "Random variable", y);
  check_positive_finite(function, "Degrees of freedom parameter", nu);
  check_consistent_sizes(function, "Random variable", y,
                         "Degrees of freedom parameter", nu);

  scalar_seq_view<T_y> y_vec(y);
  scalar_seq_view<T_dof> nu_vec(nu);
  size_t N = max_size(y, nu);

  operands_and_partials<T_y, T_dof> ops_partials(y, nu);

  // Explicit return for extreme values
  // The gradients are technically ill-defined, but treated as zero
  for (size_t i = 0; i < stan::length(y); i++) {
    if (value_of(y_vec[i]) == 0) return ops_partials.build(0.0);
  }

There are two main differences: there's an if statement that returns and there's an if embedded inside a for loop that also returns. I just find it much easier to follow the intent of the code in the first over the second. I can't pinpoint a reason why.

@seantalts, I'd like to end this discussion. Here's what I propose: if either Bob, Ben, or you find it difficult to understand the code with the conditional and for loops split on two lines, then we go with it on a single line. If that's not true, then we go with it split on two lines.

Member

syclik commented Nov 8, 2017

I gave it more thought. Something to address up front: I really hope we don't treat process/tooling as something that's frozen forever. We own it. We should be able to change it to help us out as we move forward. That said, I don't want to revisit this again for a long time.

On to the two flags I'm suggesting we change: I'm still requesting they be turned to false. I don't think I clearly articulated the issue. It seems like Krzysztof and myself find this to be a readability issue, not a cosmetic issue. I think this is asymmetric, but please let me know if I'm mistaken. Bob, Ben, or Sean, if conditionals and loops are on two lines, do you find it hard to read or hard to understand the code? For whatever reason, I find it difficult to follow the conditionals and loops when they're on the same line. I'm sure I can learn to handle it the other way, but if it's just a matter of aesthetics, then I think it's better for the team overall if we help readability. (If it were the other way around where someone found it to be hard to comprehend one way and my reason for wanting another was cosmetic, I'd gladly go along the other way for the team.)

Here's a concrete example that, for whatever reason, I find easier to read with it the first way instead of the second. From chi_square_cdf.hpp:

The way I'm requesting:

template <typename T_y, typename T_dof>
typename return_type<T_y, T_dof>::type chi_square_cdf(const T_y& y,
                                                      const T_dof& nu) {
  static const std::string function = "chi_square_cdf";
  typedef
      typename stan::partials_return_type<T_y, T_dof>::type T_partials_return;

  T_partials_return cdf(1.0);

  if (!(stan::length(y) && stan::length(nu)))
    return cdf;

  check_not_nan(function, "Random variable", y);
  check_nonnegative(function, "Random variable", y);
  check_positive_finite(function, "Degrees of freedom parameter", nu);
  check_consistent_sizes(function, "Random variable", y,
                         "Degrees of freedom parameter", nu);

  scalar_seq_view<T_y> y_vec(y);
  scalar_seq_view<T_dof> nu_vec(nu);
  size_t N = max_size(y, nu);

  operands_and_partials<T_y, T_dof> ops_partials(y, nu);

  // Explicit return for extreme values
  // The gradients are technically ill-defined, but treated as zero
  for (size_t i = 0; i < stan::length(y); i++) {
    if (value_of(y_vec[i]) == 0)
      return ops_partials.build(0.0);
  }

The way it is in this PR:

template <typename T_y, typename T_dof>
typename return_type<T_y, T_dof>::type chi_square_cdf(const T_y& y,
                                                      const T_dof& nu) {
  static const std::string function = "chi_square_cdf";
  typedef
      typename stan::partials_return_type<T_y, T_dof>::type T_partials_return;

  T_partials_return cdf(1.0);

  if (!(stan::length(y) && stan::length(nu))) return cdf;

  check_not_nan(function, "Random variable", y);
  check_nonnegative(function, "Random variable", y);
  check_positive_finite(function, "Degrees of freedom parameter", nu);
  check_consistent_sizes(function, "Random variable", y,
                         "Degrees of freedom parameter", nu);

  scalar_seq_view<T_y> y_vec(y);
  scalar_seq_view<T_dof> nu_vec(nu);
  size_t N = max_size(y, nu);

  operands_and_partials<T_y, T_dof> ops_partials(y, nu);

  // Explicit return for extreme values
  // The gradients are technically ill-defined, but treated as zero
  for (size_t i = 0; i < stan::length(y); i++) {
    if (value_of(y_vec[i]) == 0) return ops_partials.build(0.0);
  }

There are two main differences: there's an if statement that returns and there's an if embedded inside a for loop that also returns. I just find it much easier to follow the intent of the code in the first over the second. I can't pinpoint a reason why.

@seantalts, I'd like to end this discussion. Here's what I propose: if either Bob, Ben, or you find it difficult to understand the code with the conditional and for loops split on two lines, then we go with it on a single line. If that's not true, then we go with it split on two lines.

@seantalts

This comment has been minimized.

Show comment
Hide comment
@seantalts

seantalts Nov 8, 2017

Member

I think this decision criteria generally speaking is too subjective - it relies on someone assigning value judgements about formatting decisions (whether or not they are "readability issues" or "aesthetic issues"). I would prefer a simple mechanical thing like a poll ("Should these parameters be true or false - yes no don't care") if it's alright with you as I think that will set a better precedent for these decisions going forward (though ideally we punt to some industry standard in the future).

Member

seantalts commented Nov 8, 2017

I think this decision criteria generally speaking is too subjective - it relies on someone assigning value judgements about formatting decisions (whether or not they are "readability issues" or "aesthetic issues"). I would prefer a simple mechanical thing like a poll ("Should these parameters be true or false - yes no don't care") if it's alright with you as I think that will set a better precedent for these decisions going forward (though ideally we punt to some industry standard in the future).

@sakrejda

This comment has been minimized.

Show comment
Hide comment
@sakrejda

sakrejda Nov 8, 2017

Contributor
... it relies on someone assigning value judgements about formatting decisions (whether or not they are "readability issues" or "aesthetic issues"). 

Readability is not a value judgement since it's measurable. 5-15% of people (if you believe whatever Wikipedia cites) are measurably deficient in terms of handling reading tasks to the point they they could be classified as dyslexic. There's a range of things you can do to improve their performance and it's related to but also different from what you do to make reading easier for the typical reader. So in practice what is or looks to you like an aesthetic issue might be a readability issue to the next person and there's not much to do about that.

I think what you mean is that you don't trust people to relay their experience accurately and in a way that doesn't make devolve into flame wars over style. That's fine, but no need to get all relativist about everything.

I would prefer a simple mechanical thing like a poll ("Should these parameters be true or false - yes no don't care") if it's alright with you as I think that will s

Sounds good.

Contributor

sakrejda commented Nov 8, 2017

... it relies on someone assigning value judgements about formatting decisions (whether or not they are "readability issues" or "aesthetic issues"). 

Readability is not a value judgement since it's measurable. 5-15% of people (if you believe whatever Wikipedia cites) are measurably deficient in terms of handling reading tasks to the point they they could be classified as dyslexic. There's a range of things you can do to improve their performance and it's related to but also different from what you do to make reading easier for the typical reader. So in practice what is or looks to you like an aesthetic issue might be a readability issue to the next person and there's not much to do about that.

I think what you mean is that you don't trust people to relay their experience accurately and in a way that doesn't make devolve into flame wars over style. That's fine, but no need to get all relativist about everything.

I would prefer a simple mechanical thing like a poll ("Should these parameters be true or false - yes no don't care") if it's alright with you as I think that will s

Sounds good.

@syclik

This comment has been minimized.

Show comment
Hide comment
@syclik

syclik Nov 8, 2017

Member
Member

syclik commented Nov 8, 2017

@seantalts

This comment has been minimized.

Show comment
Hide comment
@seantalts

seantalts Nov 8, 2017

Member
Member

seantalts commented Nov 8, 2017

@syclik

This comment has been minimized.

Show comment
Hide comment
@syclik

syclik Nov 8, 2017

Member
Member

syclik commented Nov 8, 2017

@syclik

This comment has been minimized.

Show comment
Hide comment
@syclik

syclik Nov 8, 2017

Member
Member

syclik commented Nov 8, 2017

@bob-carpenter

This comment has been minimized.

Show comment
Hide comment
@bob-carpenter

bob-carpenter Nov 8, 2017

Contributor
Contributor

bob-carpenter commented Nov 8, 2017

@bob-carpenter

This comment has been minimized.

Show comment
Hide comment
@bob-carpenter

bob-carpenter Nov 8, 2017

Contributor
Contributor

bob-carpenter commented Nov 8, 2017

@syclik

syclik approved these changes Dec 14, 2017

LGTM! I looked at some of the diffs by hand.

@bob-carpenter

This comment has been minimized.

Show comment
Hide comment
@bob-carpenter

bob-carpenter Dec 14, 2017

Contributor

I also audited the output by hand and I can live with it, especially if it's all automated so I don't have to think much about it.

Contributor

bob-carpenter commented Dec 14, 2017

I also audited the output by hand and I can live with it, especially if it's all automated so I don't have to think much about it.

@syclik syclik merged commit c36cfa2 into develop Dec 16, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
default Build finished.
Details

@syclik syclik deleted the feature/0661-automated-style2 branch Dec 16, 2017

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