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

Only allow fvar constructor for convertible types #425

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

wgledbetter
Copy link
Contributor

#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 to boost::has_right_shift required by boost::lexical_cast.

@wgledbetter
Copy link
Contributor Author

Added overload of operator>> that behaves like operator= to solve issue with has_right_shift.

Copy link
Collaborator

@pulver pulver left a 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.

@NAThompson NAThompson self-requested a review August 27, 2020 13:10
@NAThompson
Copy link
Collaborator

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 lexical_cast much better than I do.

@NAThompson
Copy link
Collaborator

NAThompson commented Aug 27, 2020

@pulver : How do I get the cool "Approved" button you put on the PR? (This is the sort of thing I have time for!)

@pulver
Copy link
Collaborator

pulver commented Aug 27, 2020

have they failed in any other builds?

Yes: https://ci.appveyor.com/project/jzmaddock/math/builds/34818710
This is from a few days ago prior to any commits related to this PR.

How do I get the cool "Approved" button you put on the PR?

Files changed tab > green Review changes button > Approve radio button.

@jzmaddock
Copy link
Collaborator

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.

@jzmaddock
Copy link
Collaborator

I'm hoping this will fix the outstanding develop test failure: 526fc6e

@wgledbetter
Copy link
Contributor Author

@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.

@NAThompson
Copy link
Collaborator

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?

@wgledbetter
Copy link
Contributor Author

@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.

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"
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.

4 participants