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

fix assignment for nullptr var_value<matrix> and for assigning expressions #2978

Merged
merged 14 commits into from
Dec 1, 2023

Conversation

SteveBronder
Copy link
Collaborator

Summary

Fixes #2977 by forcing arena_matrix to perform a hard copy when using operator= from expressions made from other var matrices.

Tests

Tests have been added to check assigning to a var_value<Matrix> when the inner vari is nullptr and for var_value<Matrix> when the rhs is an expression and the lhs is just initialized to NaN. Tests. can be run with

python3 ./runTests.py ./test/unit/math/rev/core/var_test.cpp

Side Effects

Nope

Release notes

Bugfix for uninitialized var_value<Matrix> types and assignment when the rhs of the assignment is an expression

Checklist

  • Copyright holder: Simon's Foundation

    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

@WardBrian
Copy link
Member

WardBrian commented Nov 28, 2023

@SteveBronder this sounds kind of similar to #2895, which we 'fixed' in stan-dev/stanc3#1323.

Do we think stan-dev/stanc3#1323 is still necessary, or does this also resolve the older issue?

Also, does this resolve stan-dev/stan#3244? Edit: it does

Copy link
Member

@WardBrian WardBrian left a comment

Choose a reason for hiding this comment

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

A few small things but overall looks good. I've also plugged this branch into some downstream tests of models and confirmed it is working there now

stan/math/rev/core/var.hpp Outdated Show resolved Hide resolved
stan/math/rev/core/arena_matrix.hpp Outdated Show resolved Hide resolved
x_ans_adj(i) = -(i + 0.1);
}
EXPECT_MATRIX_EQ(x.adj(), x_ans_adj);
EXPECT_MATRIX_EQ(x_ans_adj, y.adj());
Copy link
Member

Choose a reason for hiding this comment

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

Can you a comment just stating that this is intentionally different from the above test and why (e.g., that y did not exist before and therefore does not have its own memory which can be set to 0)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, I also wrote another test here to show that the assign_nan test has the same behavior for both kinds of matrices

test/unit/math/rev/core/var_test.cpp Outdated Show resolved Hide resolved
stan/math/opencl/matrix_cl.hpp Outdated Show resolved Hide resolved
Copy link
Member

@WardBrian WardBrian left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks!

@SteveBronder SteveBronder merged commit f627912 into develop Dec 1, 2023
7 checks passed
@WardBrian WardBrian deleted the fix/hard-copy-var-assign branch December 1, 2023 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants