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

Issue #50: boolean functions - 3 #1202

Open
wants to merge 19 commits into
base: develop
from

Conversation

Projects
None yet
5 participants
@ode33
Copy link
Collaborator

commented Apr 16, 2019

Summary

Adds test functions with bool return statements rather then calling domain_error like existing check_* functions. These functions are being deployed to help improve developer productivity when building new features in stan-dev/math.

#50

Complements an earlier PR with further development in ./prim/mat/err/ and ./prim/scal/err and related test files. There is another batch still in progress.

Once this phase is completed a review of the check_* functions will commence looking into incorporating the is_* functions in a way which improves those domain_error calling functions messages.

I believe that this PR completes the objectives of Issue #50.

Some of the files touched in this PR may not be directly related to the is_* functions being created and are limited to minor syntax alterations.

Tests

Each function included has its own test file modeled off the existing tests used for the check functions. In my environment all of these replicated tests pass as expected.

Side Effects

None applicable.

Checklist

  • Math issue #(50)

  • Copyright Holder: Oliver Dechant

By submitting this pull request, the copyright holder is agreeing to 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 (make doxygen, make test-headers, ./runTests.py tests/unit/math/)

  • the code is written in idiomatic C++ and changes are documented in the doxygen

  • the new changes are tested

Oliver Dechant

@ode33 ode33 requested a review from bob-carpenter Apr 16, 2019

@ode33

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 16, 2019

I'm not clear as to why this test is causing Jenkins to fail. The culprit is_not_nan passes the given test case on my computer. Additionally it passed and was merged in the prior PR #1187. So none of the new includes in this batch should be influencing its functioning.

The only thing I notice now in is_not_nan.hpp is the extraneous #include <stan/math/prim/scal/meta/is_vector_like.hpp> which should be removed but I don't suspect that is causing problems with Jenkins.

Here is the test case

#include <stan/math/prim/mat.hpp>
#include <gtest/gtest.h>
#include <limits>

TEST(ErrorHandlingMatrix, isNotNanEigenRow) {
  stan::math::vector_d y;
  y.resize(3);

  EXPECT_TRUE(stan::math::is_not_nan(y));
  EXPECT_TRUE(stan::math::is_not_nan(y)); <-- this test fails

  y(1) = std::numeric_limits<double>::quiet_NaN();
  EXPECT_FALSE(stan::math::is_not_nan(y));
  EXPECT_FALSE(stan::math::is_not_nan(y));
}

It's tempting to comment out the test but I doubt that is a good solution.

@rok-cesnovar

This comment has been minimized.

Copy link
Collaborator

commented Apr 16, 2019

Can we really be sure that

stan::math::vector_d y;
y.resize(3);

will never have NaNs? I dont think we want uninitialized values in tests. There were (or may still be) some cases of uninitalized vectors that were causing random errors, so we should avoid this.

@ode33

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 16, 2019

I agree that we cannot. The error is of my own making.

What was confusing was that I reviewed the last commit and there is only one is_not_nan.hpp which is in /prim/scal/err/.

The error for the test case is that there also exists one new test file: is_non_nan_test which was based on check_not_nan. Both refer to respective /mat/err/[*].hpp files which do not exist! So I was confusing the merged is_not_nan from scal and the one from mat.

As a matter of fact there exist a couple more check tests [0] which do not have corresponding .hpps and they seemed innocuous so I ignored them and didn't intend to have them replicated as is tests but overlooked one.

[0] https://github.com/stan-dev/math/blob/develop/test/unit/math/prim/mat/err/check_not_nan_test.cpp

Something seems to have been caught by Jenkins for the is copy of one of these, specifically the is_not_nan_test.

Since there is no proper calling function it is right to delete the blocking test here. Why exactly it blocked the build while the others do not I can't guess.

Should the other redundant but innocuous test files be removed? They may still be useful at some point.

@bob-carpenter

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

Is @rok-cesnovar going to review this or should I?

We're trying not to double-review things so PR authors get consistent feedback.

@rok-cesnovar

This comment has been minimized.

Copy link
Collaborator

commented Apr 17, 2019

I would suggest you do it Bob. If I am not mistaken you also reviewed the previous boolean functions PRs, so that might help with consistency.

@bob-carpenter

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

ode33 and others added some commits Apr 17, 2019

@ode33

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 18, 2019

I'm not sure why Jenkins did this after a minor typo edit to a previously and now still passing test.

+ make -jnull test-headers
make: the '-j' option requires a positive integer argument

Otherwise I'm certain this request should build. Regardless I just merged upstream to restart the CI tests.

@bob-carpenter
Copy link
Contributor

left a comment

Thanks for taking on this massive project.

I'm running out of steam about halfway through here and am hoping you can apply some of the general principles throughout and I can just take a pass through the whole thing again.

It might be easier going forward to use smaller pull requests to iron out general principles before applying them everywhere like this. I feel bad asking for so many changes at this late stage.

Show resolved Hide resolved stan/math/prim/mat/err/check_cov_matrix.hpp Outdated
Show resolved Hide resolved stan/math/prim/mat/err/check_pos_definite.hpp
Show resolved Hide resolved stan/math/prim/mat/err/is_consistent_size_mvt.hpp Outdated
Show resolved Hide resolved stan/math/prim/mat/err/is_consistent_sizes_mvt.hpp Outdated
Show resolved Hide resolved stan/math/prim/mat/err/is_consistent_sizes_mvt.hpp
Show resolved Hide resolved stan/math/prim/scal/err/is_consistent_sizes.hpp Outdated
Show resolved Hide resolved stan/math/prim/scal/err/is_consistent_sizes.hpp Outdated
Show resolved Hide resolved stan/math/prim/scal/err/is_consistent_sizes.hpp
Show resolved Hide resolved stan/math/prim/scal/err/is_consistent_sizes.hpp
* @return <code>true</code> if <code>y</code> is greater than low and if no
* element of <code>y</code> or <code>low</code> is NaN.
*/
template <typename T_y, typename T_low>

This comment has been minimized.

Copy link
@bob-carpenter

bob-carpenter Apr 19, 2019

Contributor

When is a lower bound going to be vectorized like this? I'm also confused by what look like two different bodies here, one treating y like a scalar and one treating it like a vector. If the get() applied to scalars, isn't this just testing the same thing twice since the first loop seems to assume a scalar?

@drezap drezap referenced this pull request Apr 19, 2019

Open

testing/Missing Prim Tests #1194

10 of 18 tasks complete
@ode33

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 20, 2019

Thanks for taking on this massive project.

I'm running out of steam about halfway through here and am hoping you can apply some of the general principles throughout and I can just take a pass through the whole thing again.

It might be easier going forward to use smaller pull requests to iron out general principles before applying them everywhere like this. I feel bad asking for so many changes at this late stage.

It's been a great opportunity to learn about the structure of a large C++ project. As of today I have a last final exam Tuesday morning and I need to allocate the bulk of my weekend towards that so I hope to make incremental progress later into the nights (if at all) until then.

There is certainly a lot of repetition here and I intended this to be one of three smaller PR segments but for whatever reasons this ended up including quite a lot of changes. It should be possible to split this into two PRs.

I have no problem making whatever changes you need to feel comfortable about code quality. It's been your review and suggestions which help me the most. I responded to @syclik and asked for a project that wasn't time dependent so I've appreciated the flexibility to pick away at this over the past 3 months of my sophomore year. I have no problem continuing for however much longer is necessary for this to be beneficial to the developer team and consistent with the quality of the rest of the project.

@bob-carpenter

This comment has been minimized.

Copy link
Contributor

commented Apr 21, 2019

Oliver Dechant and others added some commits May 8, 2019

Oliver Dechant
oliver: code fixes
	modified:   stan/math/prim/mat.hpp
	modified:   stan/math/prim/mat/err/check_finite.hpp
	modified:   stan/math/prim/mat/err/check_pos_semidefinite.hpp
	modified:   stan/math/prim/mat/err/check_spsd_matrix.hpp
	modified:   stan/math/prim/mat/err/is_consistent_size_mvt.hpp
	modified:   stan/math/prim/mat/err/is_lower_triangular.hpp
	modified:   stan/math/prim/mat/err/is_mat_finite.hpp
	modified:   stan/math/prim/mat/err/is_pos_semidefinite.hpp
	modified:   stan/math/prim/mat/err/is_positive_ordered.hpp
	modified:   stan/math/prim/mat/err/is_simplex.hpp
	modified:   stan/math/prim/mat/err/is_spsd_matrix.hpp
@serban-nicusor-toptal

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

@ode33 Hey, can you push a commit now ? ( just do a new line ) .
Thanks!

PS: Jenkins went through a migration process

@ode33

This comment has been minimized.

Copy link
Collaborator Author

commented May 31, 2019

@serban-nicusor-toptal Yes I can later tonight on my desktop. Thanks!

@syclik syclik self-requested a review Jun 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.