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

Cleanup some of the C++ #871

Merged

Conversation

SteveBronder
Copy link
Contributor

Summary

This PR has a few cleanups of the C++ I've been littering around several PRs and I thought it would be better to just put them all into one PR

Description

  1. Have a newline for for loop body and ending curly braces

I always put the C++ through clang-format before reading it so that the end curly brace of for loop was on a new line. This fixes that so the for loop and body end curly braces go on new lines with proper indentation.

  1. Removing the use of to_vec()

to_vec() was used when validating dimensions and all it did was make an std::vector<size_t> of the given dimensions. So instead we just do this inline using std::vectors initializer list syntax.

So this

      context__.validate_dims("data initialization","sigma","double",
          context__.to_vec(J));

becomes

      context__.validate_dims("data initialization","sigma","double",
           std::vector<size_t>{static_cast<size_t>(J)});
  1. Remove the .clear() and push backs from get_param_names()

For get_param_names(std::vector<std::string>& names__) we would call names__.clear() and then push strings into the vector, but we can just do this in one line

names__ = std::vector<std::string>{"mu", "tau", "theta_tilde", "theta"};

and we do the same thing for get_dims()

    dimss__ = std::vector<std::vector<size_t>>{std::vector<size_t>{},
      std::vector<size_t>{}, std::vector<size_t>{static_cast<size_t>(J)},
      std::vector<size_t>{static_cast<size_t>(J)}};
  1. Use + and strings instead of a stringstream and << for the json output methods like get_constrained_sizedtypes()

  2. Add all the num_params_r__ in one line.

Instead of accumulating the num_params_r__ on each line we do it all on one line like

    num_params_r__ = 1 + 1 + J;

Instead of

    num_params_r__ = 0U;

    try {
      num_params_r__ += 1;
      num_params_r__ += 1;
      num_params_r__ += J;
    } catch (const std::exception& e) {
      stan::lang::rethrow_located(e, locations_array__[current_statement__]);
      // Next line prevents compiler griping about no return
      throw std::runtime_error("*** IF YOU SEE THIS, PLEASE REPORT A BUG ***"); 
    }

idt this can ever throw since we are just adding up integer values.

  1. Remove fill() from data (but leave it for transformed data)

In my evergoing quest to remove the fill's that we don't need I did figure out how to remove them from data. Though not really much of an optimization.

We are guranteed (and validate) that the size of data is correct, so we don't need to fill it with NA. However, we do need to fill transformed data with NA as users could potentially use inner assignments that could miss some cells.

Oddly, I noticed that we only fill NA's whenver we use Eigen matrices or containers of Eigen matrices, why is that? For instance an std::vector<double> will not have it's values fill with NA. Should we be doing it always? Should we never be doing it? The safer thing is to always do it for any container idt that's a bad idea.

I removed those fills by adding a bool to the tuple that pp_set_size takes in. This feels like kind of a silly approach, I'd much rather have an optional parameter but I couldn't figure out how to do that.

  1. Cleans up the log_prob and write_array functions that only take in a vector of reals and not a vector of ints so they are a bit more efficient.

Release notes

Cleanup readability of the C++

Copyright and Licensing

By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the BSD 3-clause license (https://opensource.org/licenses/BSD-3-Clause)

Steve Bronder

Copy link
Member

@rok-cesnovar rok-cesnovar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. One comment typo I think and then should be good to go.

},
"block": "parameters"
} |}]

(*Adds a backslash to all the inner quotes and then
unblash the ones near a plus*)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unslash? :)

pos__ = (pos__ + 1);}}
pos__ = (pos__ + 1);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤘 🤘 🤘

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was bothering me for so long lol

@SteveBronder
Copy link
Contributor Author

Cool beans, fixed! thanks for the review!

@SteveBronder SteveBronder merged commit 8626779 into stan-dev:master Apr 7, 2021
@SteveBronder SteveBronder deleted the feature/cleanup-cpp-meta-info branch April 8, 2021 01:33
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

Successfully merging this pull request may close these issues.

2 participants