Skip to content
This repository has been archived by the owner on Dec 27, 2023. It is now read-only.

Remove hardcoded dependency #64

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

Conversation

Blackhol3
Copy link

It is currently impossible to work with trigonometric functions (cos(), sin(), …) with more than 32 digits of precision, since the calls to DecimalConstants, where pi() is limited to 32 digits, are hardcoded.

This PR allows to extend Decimal and to modify the value of Decimal::DECIMAL_CONSTANTS in order to change the class being called when numeric constants are needed.

@castarco
Copy link
Contributor

I'll try to review this PR during this week. But my first diagonal reading gave me some simple insight that I want to share:

  • The PR should be smaller, it tries to change too many things at the same time.

In addition to that... I'm not proud at all for the architecture of this library, and I think it need a better (& simpler replacement). This library tried to cover very different use cases (scientific computation, symbolic computation, & even financial computation), and that was a big mistake.

@Blackhol3
Copy link
Author

If you feel this PR is too big, the commits 66d636b and 3506a9c, although related, are rather independent and can be left aside at the moment.

About the architecture… Well, at least you're aware of the problem ^^. I've read somewhere that you care a lot about the performance of this library, so what I've proposed here is quite a hack, but it is probably the fastest way to go without reorganising the whole thing.

If you want some inspiration for the structure of this project, you can maybe check brick/math. I'm still hesitating between this two libraries, to be honest. Both have their advantages. Anyway, if you want some help for the refactoring, do not hesitate to share your thoughts. I'll be happy to give a hand!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants