Skip to content

Comments

Move size_zero() calls after other consistency checks (A-I)#1758

Merged
bob-carpenter merged 2 commits intodevelopfrom
cleanup/1757-move-size-zero-checks_p1
Mar 5, 2020
Merged

Move size_zero() calls after other consistency checks (A-I)#1758
bob-carpenter merged 2 commits intodevelopfrom
cleanup/1757-move-size-zero-checks_p1

Conversation

@mcol
Copy link
Member

@mcol mcol commented Mar 4, 2020

Summary

This is the first of two PRs that deals with moving the size_zero calls after the other consistency checks on function arguments. This covers all distributions with names starting A-I.

This also improves the consistency of the placement of the using std:: statements and the declaration of the operands_and_partials variable. This came about as in many cases these lines of code were in between the size_zero checks and the other consistency checks, so they had to be moved anyway. Now all distribution files look very similar in their first few chunks, which is a small added benefit, I think.

Tests

No new tests. The distribution test framework already contains tests for input vectors of size zero (search for test_length_0_vector).

Side Effects

None.

Checklist

  • Math issue Move size_zero checks after consistency checks on arguments #1757

  • Copyright holder: Marco Colombo

    The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
    - Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
    - Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)

  • the basic tests are passing

    • unit tests pass (to run, use: ./runTests.py test/unit)
    • header checks pass, (make test-headers)
    • dependencies checks pass, (make test-math-dependencies)
    • docs build, (make doxygen)
    • code passes the built in C++ standards checks (make cpplint)
  • the code is written in idiomatic C++ and changes are documented in the doxygen

  • the new changes are tested

@stan-buildbot
Copy link
Contributor


Name Old Result New Result Ratio Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan 5.04 4.8 1.05 4.79% faster
low_dim_corr_gauss/low_dim_corr_gauss.stan 0.02 0.02 1.01 1.23% faster
eight_schools/eight_schools.stan 0.09 0.09 0.96 -3.95% slower
gp_regr/gp_regr.stan 0.22 0.22 0.99 -0.68% slower
irt_2pl/irt_2pl.stan 6.37 6.53 0.98 -2.39% slower
performance.compilation 89.23 86.16 1.04 3.44% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan 7.81 7.68 1.02 1.58% faster
pkpd/one_comp_mm_elim_abs.stan 20.97 20.82 1.01 0.73% faster
sir/sir.stan 96.59 96.76 1.0 -0.17% slower
gp_regr/gen_gp_data.stan 0.05 0.05 0.99 -1.01% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan 3.09 2.97 1.04 3.84% faster
pkpd/sim_one_comp_mm_elim_abs.stan 0.31 0.32 0.98 -1.82% slower
arK/arK.stan 1.78 1.74 1.03 2.57% faster
arma/arma.stan 0.68 0.66 1.02 2.28% faster
garch/garch.stan 0.58 0.58 1.0 -0.48% slower
Mean result: 1.0072739521

Jenkins Console Log
Blue Ocean
Commit hash: d0fe7ec


Machine information ProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010

CPU:
Intel(R) Xeon(R) CPU E5-1680 v2 @ 3.00GHz

G++:
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/usr/include/c++/4.2.1
Apple LLVM version 7.0.2 (clang-700.1.81)
Target: x86_64-apple-darwin15.6.0
Thread model: posix

Clang:
Apple LLVM version 7.0.2 (clang-700.1.81)
Target: x86_64-apple-darwin15.6.0
Thread model: posix

@mcol
Copy link
Member Author

mcol commented Mar 5, 2020

It turns out that moving the size_zero checks exposed a bug in check_bounded, in that it didn't handle correctly size zero vectors (or rather, scalar_seq_view doesn't, but fixing check_bounded has fewer performance implications). With that simple fix, all tests pass, and this is ready for review.

Copy link
Member

@bob-carpenter bob-carpenter left a comment

Choose a reason for hiding this comment

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

Thanks so much for the heroic amount of error behavior fixes here. I was exhausted just after scrolling to check the new orders were right. And thanks also for fixing so much of the doc and code formatting around these changes.

return 0;
}

using std::atan;
Copy link
Member

Choose a reason for hiding this comment

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

[comment]
Just a comment because you're fixing so much of the style, but not worth going in and changing for this PR.

I'd like to see using statements either all piled together at the top of the function or pushed down to just before the symbol is used.

Copy link
Member Author

Choose a reason for hiding this comment

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

Up to now they were placed inconsistently, at least my change put them in the same place. I can move them to the top in the future.

* @param mu (Sequence of) location(s).
* @param sigma (Sequence of) scale(s).
* @return The log of the product of densities.
* @tparam T_scale Type of scale.
Copy link
Member

Choose a reason for hiding this comment

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

[comment]
We want to head toward lower case and no periods for phrases that are not complete sentences.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's where I'm heading to. I think what happened here is that I started fixing this file, then noticed it was one of the deprecated ones and forgot to undo completely my changes.

@bob-carpenter bob-carpenter merged commit fb2bb79 into develop Mar 5, 2020
@bob-carpenter bob-carpenter deleted the cleanup/1757-move-size-zero-checks_p1 branch March 5, 2020 15:52
@mcol mcol linked an issue Mar 5, 2020 that may be closed by this pull request
@bob-carpenter
Copy link
Member

bob-carpenter commented Mar 5, 2020 via email

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.

Move size_zero checks after consistency checks on arguments

3 participants