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

Adding functions to real128 #140

Merged
merged 8 commits into from
May 7, 2018
Merged

Conversation

darioizzo
Copy link
Contributor

real128 was missing some functions (e.g. tan, erf etc..) that are found necessary to answer, for example, to the cancellation issue in darioizzo/audi#29

in this PR tan, inverse trig, erf are added to allow the class to be used to build a quad precision differential algebra (audi)

@@ -6,5 +6,8 @@
"doc/doxygen/latex/": true,
"doc/doxygen/xml/": true,
"doc/sphinx/_build/": true
},
Copy link
Owner

Choose a reason for hiding this comment

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

Could you please remove this change?

}
/// In-place lgamma function
/**
* This method will set \p this to the value of the natural logarithm of its gamma function
Copy link
Owner

Choose a reason for hiding this comment

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

A period is missing at the end here.


/** @} */

/** @defgroup real128_trig real128_trig
Copy link
Owner

Choose a reason for hiding this comment

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

This should probably be real128_hyper rather than real128_trig? The doxygen group should then also be imported in the sphinx documentation, in doc/sphinx/real128.rst (the same way it's done for the trigonometric functions, for instance).

return x.erf();
}

/// Natural logarithm of the gamma funcion
Copy link
Owner

Choose a reason for hiding this comment

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

In the real class, gamma functions and related are grouped into their own group. E.g., see:

https://bluescarni.github.io/mppp/real.html#gamma-functions

Could you do the same with real128's lgamma function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noted you use gamma and not tgamma as a name for the gamma function (as opposed to whats in quadmath and std). Is there a reason?

@bluescarni
Copy link
Owner

Thanks, it looks good! I just have some minor doc nitpicks.

@darioizzo
Copy link
Contributor Author

darioizzo commented May 4, 2018

doing this PR I developed some questions I will log here, they may be irrelevant:

  • There is not template specialisation of std::numeric_limits. This would allows for example to use boost generic classes such as bernoulli numbers, numerical constants etc.... also with mppp classes. I used the audi project (for real128) to test and indeed improves the interoperability with boost and std algorithms.

  • Serialization is not implemented, probably left to the user if needed, I guess

  • piranha v0.10 seems to not to instatiate subs method: error: no matching function for call to ‘piranha::polynomial<mppp::real128, piranha::monomial<short unsigned int> >::subs(const string&, mppp::real128) const to be used on mppp::real128. I may be doing something wrong too, not sure.

  • gamma function is called in mppp gamma while in quadmath and std its tgamma (true gamma)

"doc/doxygen/latex/": true,
"doc/doxygen/xml/": true,
"doc/sphinx/_build/": true
"doc/doxygen/html/": true
Copy link
Owner

Choose a reason for hiding this comment

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

@darioizzo seems like something else might have slipped in here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so also that is to be removed?
"doc/doxygen/html/": true

Copy link
Owner

Choose a reason for hiding this comment

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

No, what I mean is that the .vscode/settings.json file shouldn't show up at all in the PR diff because I don't think there are any modifications needed to it in this PR. What I complained about originally was a modification to the file which (I believe) is done automatically by VScode each time you open the workspace (because of some extension which is installed on your side I believe).

Maybe you can just copy paste the version from master?

@bluescarni
Copy link
Owner

  • The specialisation of std::numeric_limits is indeed missing. I've postponed it so far because I haven't needed it yet and I was too lazy to learn how to do it properly :) But since it improves usage with generic boost algos etc., I'll put it on the TODO list.

  • Yes serialisation is implemented only for integer at the moment (and not Boost.Serialization, just a custom serialisation format). Another thing TBD eventually, but it should be easy to add it to audi in the meantime. Just ping me if you need help, but basically just converting the real128 to string and storing that will be enough (won't be performant, but it will work).

  • I have to look into that, I don't have an immediate answer.

  • The tgamma name is a historical thing. Before the standardisation of C's math library, various implementations offered a gamma function which was actually computing the logarithm of the gamma function rather than the gamma function itself. So, in order to avoid name clashes with existing C implementations, the C committee decided to name the function tgamma (true gamma) rather than simply gamma. The quadmath library's API closely mirrors the API of the C standard library, so they also use the tgamma name. The MPFR API, however, on which the real class is based, just uses the name gamma as they are not limited by legacy concerns. In mp++ so far I have adopted the "rule" to just mirror as closely as possible the underlying API of the libraries on top of which mp++ is implemented, so I would be fine to have real128 have a tgamma function and real have a gamma function.

@bluescarni
Copy link
Owner

@darioizzo I'll go ahead and merge this, I'll fix the vscode file issue later. Thanks!

@bluescarni bluescarni merged commit 8984c1f into bluescarni:master May 7, 2018
@darioizzo
Copy link
Contributor Author

darioizzo commented May 7, 2018 via email

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.

2 participants