Skip to content

Alway use CwiseUnaryOp for .val() of var types - remove .val_op()#3351

Open
andrjohns wants to merge 5 commits into
developfrom
val-op
Open

Alway use CwiseUnaryOp for .val() of var types - remove .val_op()#3351
andrjohns wants to merge 5 commits into
developfrom
val-op

Conversation

@andrjohns

Copy link
Copy Markdown
Collaborator

Summary

The .val() custom Eigen member always returns a CwiseUnaryView when called on a non-const input (as a CwiseUnaryView can be assigned to). However the CwiseUnaryView can result in compilation errors in some Eigen expressions and so the val_op() member was introduced as a way to explicitly request a CwiseUnaryOp (normally only called for const inputs).

For var types even though the var itself is non-const, the val_ member of the underlying vari is always const - so we actually always want a CwiseUnaryOp.

This PR replaces the val_op() implementation with template overloads for the val() implementation - only returning CwiseUnaryView for fvar inputs, otherwise returning a CwiseUnaryOp.

All usages of val_op() through the codebase have also been replaced with just val()

Tests

N/A - existing tests should successfully compile and run

Side Effects

N/A

Release notes

The .val() member function always returns a CwiseUnaryOp when called with a var type, the val_op() member function has been removed

Checklist

  • Copyright holder: Andrew Johnson

    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
Copy Markdown
Member

The idea here seems sound, and your explanation of why the different functions existed in the first place is better than the doc we currently have IMO.

So, if you wouldn't mind updating that part of the doxygen to remove the mention of val_op and update our description of adj_op, I think this is good to go

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.

3 participants