-
Notifications
You must be signed in to change notification settings - Fork 225
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
Only allow fvar constructor for convertible types #425
base: develop
Are you sure you want to change the base?
Conversation
Added overload of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change looks like a good improvement in general compatibility, with minimal changes. Really appreciate the contribution @wgledbetter.
@NAThompson any comments before merging?
The AppVeyor build issues are pre-existing and seem to be independent of this change.
The build failures do look like a pre-existing problem, but unfortunately I don't have time to investigate; have they failed in any other builds? I have read the diffs and think they look good. I'm going to let @jzmaddock squash merge this since he understands |
@pulver : How do I get the cool "Approved" button you put on the PR? (This is the sort of thing I have time for!) |
Yes: https://ci.appveyor.com/project/jzmaddock/math/builds/34818710
|
Looks good to me too - do we have a test case for the original Eigen issue? We do have some Eigen interoperability tests in Multiprecision which could be copied here (which is to say I'll sort out the Jamfile stuff if you don't want to go there). I'll try to sort out the Appveyor issue too - we really need to get that fixed - not really sure how it crept in to be honest. |
I'm hoping this will fix the outstanding develop test failure: 526fc6e |
@NAThompson and @pulver Thanks for the quick review! @jzmaddock I'll work on some Eigen tests over the weekend if you can help me put them into the existing test framework. |
So there is a broader question now: This now explicitly uses Eigen in the boost source directory, but traditionally we only test that our code is "generic enough" to interoperate with Eigen. Do we have a compelling reason to use Eigen explicitly, rather than (say) writing generic code that will work with Eigen and Boost.ublas? |
@NAThompson boost multiprecision has an eigen.hpp compatibility file that directly includes Eigen, and I was referencing that code for the recent updates. It provides an overload of Eigen::NumTraits that is necessary for full compatibility. The other overload it provides is Eigen::ScalarBinaryOpTraits. This one is proving troublesome because the libeigen3-dev package on travis:xenial is out of date. That reason alone is probably enough to remove the Eigen include, since end-users may still be using the older version. The commit I'm about to push relocates the Eigen code from the source directory to the test directory. |
752436e
to
ab4b761
Compare
Only test quadrature constexpr if compatibility macro typo Fix derivative casting? different way boost::has_right_shift fails for me even before fvar changes... prep for PR cleaning
Quicker testing fix travis prep for PR
match parentheses Set active variables still debugging using no typename catch all typename eigen built-in common functions element-wise ops scalar binary op traits cast for multiplication try float specialization is travis' eigen up to date? Newer eigen, relocate compatibility code relocate eigen code location add typename and static much faster debugging help type conversion fix cast include for determinant and inverse fix dimension tolerance tweak tolerances osx cleanup preliminary eigen tests debug eigen tests match parentheses Set active variables still debugging using no typename catch all typename eigen built-in common functions element-wise ops scalar binary op traits cast for multiplication try float specialization is travis' eigen up to date? Newer eigen, relocate compatibility code relocate eigen code location add typename and static much faster debugging help type conversion fix cast include for determinant and inverse fix dimension tolerance tweak tolerances osx cleanup try fixing windows build Try this eigen location undo no 'cl' command? Is it because of "include?" Try "vcpkg integrate install"
#424
Use SFINAE to enable copy construction and assignment only from valid types. Allows use in Eigen library.
test_autodiff
1, 2, and 3 succeed, but number 4 fails due toboost::has_right_shift
required byboost::lexical_cast
.