Skip to content

feature: hyperbolic trig functions#348

Open
kayarre wants to merge 3 commits intolibfive:masterfrom
kayarre:feature/hyperbolic
Open

feature: hyperbolic trig functions#348
kayarre wants to merge 3 commits intolibfive:masterfrom
kayarre:feature/hyperbolic

Conversation

@kayarre
Copy link

@kayarre kayarre commented May 27, 2020 •
edited
Loading

I have attempted to implement hyperbolic functions in libfive.

I am rather unsure of what all I actually needed to change.
Currently it compiles, but I haven't written any tests, and I am unsure of the correct bounds.
Additionally it appears that c++11 flag is required from eigen

see:#347

mkeeter
mkeeter reviewed May 30, 2020
View reviewed changes
mkeeter
mkeeter reviewed May 30, 2020
View reviewed changes
mkeeter
mkeeter reviewed May 30, 2020
View reviewed changes
@mkeeter
Copy link
Member

mkeeter commented May 30, 2020

Thanks for the new operations!

This generally looks good, and I put a few in-line comments in the PR.

To get these operations tested (at a very basic level), you should edit the "Every operation" test for the various evaluators, e.g. here, here, and here

For cleanliness, you could change the loop to something like

for (unsigned i=0; i < LAST_OP; ++i) {
    auto op = (libfive::Opcode::Opcode)i;
    if (Opcode::args(op) <= 0) {
        continue;
    }
    // previous contents of test

This will get your new functions evaluated, while skipping opcodes that can't be constructed naturally. You could also add a few spot checks, using the Approx wrapper, e.g. how it's done here.

@kayarre kayarre changed the title WIP feature: hyperbolic trig functions feature: hyperbolic trig functions Jun 5, 2020
@kayarre
Copy link
Author

kayarre commented Jun 5, 2020

I added the testing, although I couldn't quite interpret what to check for in the derivatives, but came up with some simple smell tests, based on what'ts already there.

@mkeeter
Copy link
Member

mkeeter commented Jun 7, 2020

This generally looks good, but I'm having trouble building it locally:

It looks like

CCACHE_CPP2=yes CCACHE_HASHDIR=yes /usr/local/bin/ccache /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++  -Dfive_EXPORTS -I../libfive/src/../include -isystem /usr/local/include -isystem /usr/local/Cellar/eigen/3.3.7/include/eigen3 -Wall -Wextra -g -fPIC -pedantic -Werror=switch -march=native -stdlib=libc++ -O3 -DNDEBUG -O3 -DRELEASE -DEIGEN_NO_DEBUG -fPIC   -std=gnu++11 -MD -MT libfive/src/CMakeFiles/five.dir/eval/eval_array.cpp.o -MF libfive/src/CMakeFiles/five.dir/eval/eval_array.cpp.o.d -o libfive/src/CMakeFiles/five.dir/eval/eval_array.cpp.o -c ../libfive/src/eval/eval_array.cpp
../libfive/src/eval/eval_array.cpp:344:19: error: no matching function for call to 'asinh'
            out = asinh(a);
                  ^~~~~
/usr/include/math.h:355:15: note: candidate function not viable: no known conversion from 'Eigen::DenseBase<Eigen::Block<Eigen::Array<float, -1, 256, 1, -1, 256>, 1, 256, true> >::SegmentReturnType' (aka 'VectorBlock<Eigen::Block<Eigen::Array<float, -1, 256, 1, -1, 256>, 1, 256, true> >') to 'double' for 1st argument
extern double asinh(double);
              ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/math.h:1048:46: note: candidate function not viable: no known conversion from 'Eigen::DenseBase<Eigen::Block<Eigen::Array<float, -1, 256, 1, -1, 256>, 1, 256, true> >::SegmentReturnType' (aka 'VectorBlock<Eigen::Block<Eigen::Array<float, -1, 256, 1, -1, 256>, 1, 256, true> >') to 'float' for 1st argument
inline _LIBCPP_INLINE_VISIBILITY float       asinh(float __lcpp_x) _NOEXCEPT       {return ::asinhf(__lcpp_x);}
                                             ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/math.h:1049:46: note: candidate function not viable: no known conversion from 'Eigen::DenseBase<Eigen::Block<Eigen::Array<float, -1, 256, 1, -1, 256>, 1, 256, true> >::SegmentReturnType' (aka 'VectorBlock<Eigen::Block<Eigen::Array<float, -1, 256, 1, -1, 256>, 1, 256, true> >') to 'long double' for 1st argument
inline _LIBCPP_INLINE_VISIBILITY long double asinh(long double __lcpp_x) _NOEXCEPT {return ::asinhl(__lcpp_x);}
                                             ^
../libfive/src/../include/libfive/tree/tree.hpp:235:10: note: candidate function not viable: no known conversion from 'Eigen::DenseBase<Eigen::Block<Eigen::Array<float, -1, 256, 1, -1, 256>, 1, 256, true> >::SegmentReturnType' (aka 'VectorBlock<Eigen::Block<Eigen::Array<float, -1, 256, 1, -1, 256>, 1, 256, true> >') to 'const libfive::Tree' for 1st argument
OP_UNARY(asinh);
         ^
../libfive/src/../include/libfive/tree/tree.hpp:222:41: note: expanded from macro 'OP_UNARY'
#define OP_UNARY(OP)      libfive::Tree OP(const libfive::Tree& a)
                                        ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/math.h:1054:1: note: candidate template ignored: requirement 'std::is_integral<VectorBlock<Block<Array<float, -1, 256, 1, -1, 256>, 1, 256, true>, -1> >::value' was not satisfied [with _A1 = Eigen::VectorBlock<Eigen::Block<Eigen::Array<float, -1, 256, 1, -1, 256>, 1, 256, true>, -1>]
asinh(_A1 __lcpp_x) _NOEXCEPT {return ::asinh((double)__lcpp_x);}

(with similar errors for acosh and atanh).

Sure enough, asinh isn't listed in Eigen's math operators. Are you building against a newer Eigen revision?

@kurtsansom
Copy link

kurtsansom commented Jun 8, 2020

You are right it's on the master branch, I didn't realize it was not released yet.
Eigen 3.3.7 is about a year and a half old now, so maybe master makes sense?

It's not clear to me when they will make another release, but I imagine these changes would be a part of that release.

#if EIGEN_HAS_CXX11_MATH
  EIGEN_ARRAY_DECLARE_GLOBAL_UNARY(asinh,scalar_asinh_op,inverse hyperbolic sine,\sa ArrayBase::asinh)

@mkeeter mkeeter added the kernel label Jun 13, 2020
@mkeeter
Copy link
Member

mkeeter commented Jun 13, 2020

I looked at pasting in the EIGEN_ARRAY_DECLARE_GLOBAL_UNARY macros, but it looks like they also require internal functions to be defined (e.g. scalar_asinh_op).

We probably shouldn't require a master build of Eigen, although it's frustrating that they haven't released in a while. I think the simplest option is either
a) Write this as a for loop instead of an array function
or
b) Switch to the arithmetic definitions for these functions (e.g. asinh(x) = log(x + sqrt(x**2 + 1)))

We use for loops elsewhere in the array evaluators (e.g. for atan2), so that's probably the most internally consistent option.

@SGSSGene
Copy link
Contributor

SGSSGene commented Feb 8, 2021

Eigen3 has made two new releases: http://eigen.tuxfamily.org/index.php?title=Main_Page
Does that solve the building issue?

@mkeeter
Copy link
Member

mkeeter commented Feb 8, 2021

Oh cool, I'll check it out!

@d01010101 d01010101 mentioned this pull request Feb 12, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mkeeter mkeeter mkeeter left review comments

Assignees

No one assigned

Labels

kernel

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@kayarre @mkeeter @kurtsansom @SGSSGene