jmh530 commented 1 year ago

Monic is a specialization of Polynomial assuming the leading coefficient equals to 1. I want to add my own version of log1p (because phobos documentation lies about how it works) and the CEPHES implementation uses the equivalent of this.

The tester fails, but I believe it is unrelated. I tested it with a separate version identifier and tests pass.

9il commented 1 year ago

The Polynomial is a high-level stuff with RC arrays. For simple things like math functions, it is better to not to allocate memory at all.

jmh530 commented 1 year ago

@9il Happy to make the required changes. I was following how Polynomial worked more than anything. The phobos version of poly doesn't make any allocations. It would be a breaking change if making mir's polynomial function work similarly (since it returns a Polynomial!F), but we could introduce a new poly function and then have Polynomial's opCall use that.

jmh530 commented 1 year ago

@9il Changes in https://github.com/libmir/mir-algorithm/pull/453/commits/fada1eebee82c4b6e12f0279ddf4b2be885d0ebd

9il commented 1 year ago

but we could introduce a new poly function and then have Polynomial's opCall use that.

Looks good. In that case, you don't need monic because it will be compiled-time optimized from poly. It has to be.

jmh530 commented 1 year ago

@9il Coefficients are parameters that can be provided at run-time. This means that the compiler-optimized version of poly has to be able to handle any value for the leading coefficient. monic should be able to avoid some calculations. Though it would take some testing to determine how much an impact that is.

jmh530 commented 1 year ago

@9il Made some changes and added the poly function. The only challenge with respect to calling poly from Polynomial.opCall would be that I can't use a core.lifetime.move. So I left the current implementation for now.

9il commented 1 year ago

overload for slice isn't needed, arrays are fine
I don't get why move may be needed. someRCArray[] returns an array view, now need to move it.
Why do we have to have monic? Why not to use poly instead?

jmh530 commented 1 year ago

overload for slice isn't needed, arrays are fine

This makes things a little more awkward when calling it with slices. If I remove those overloads, then I got a whole bunch of template errors when calling it with either poly(value, x) or poly(value, x[]). I have to do poly(value, x.array), which results in a GC allocation.

* I don't get why `move` may be needed. `someRCArray[]` returns an array view, now need to move it.

move was only used in Slice version. I only included it because it was used in the original. I don't always know when to use move or not.

* Why do we have to have monic? Why not to use poly instead?

The motivation was originally that a version of monic is used in Cephes' version of log1p, presumably because there are performance benefits (as I mentioned above, the run-time calls of the function can't optimize away the operations in poly that monic gets rid of). I can do some profiling though to check it out.

9il commented 1 year ago

The motivation was originally that a version of monic is used in Cephes' version of log1p, presumably because there are performance benefits (as I mentioned above, the run-time calls of the function can't optimize away the operations in poly that monic gets rid of). I can do some profiling though to check it out.

I am 99% sure it will be optimised. godbold.org could help.

This makes things a little more awkward when calling it with slices. If I remove those overloads, then I got a whole bunch of template errors when calling it with either poly(value, x) or poly(value, x[]). I have to do poly(value, x.array), which results in a GC allocation.

Do you mean in unit tests or your private code?

jmh530 commented 1 year ago

I am 99% sure it will be optimised. godbold.org could help.

Is there any way to include dependencies with that?

Do you mean in unit tests or your private code?

I mean adding a unittest here that makes it a little clearer how it works.

9il commented 1 year ago

Is there any way to include dependencies with that?

Yes, it allows to inclusion mir libs. However, you don't need that to check if you do a little code update.

I mean adding a unittest here that makes it a little clearer how it works.

Just use arrays.

jmh530 commented 1 year ago

Here's the godbolt: https://godbolt.org/z/cvvx48GeE

Looks like the polyImpl version uses some additional instructions.

I'm happy to just use arrays for my application of these functions. My point about lightConst UT examples was just so that I have the example of how it works somewhere so that I don't forget it. It's orthogonal to using arrays...

9il commented 1 year ago

Looks like the polyImpl version uses some additional instructions.

https://godbolt.org/z/Mj1q7rf1h

jmh530 commented 1 year ago

@9il So it looks like if polyImpl is used within another function and the compiler knows about the parameters, then it will inline everything away. I suppose that is good enough for my case.

jmh530 commented 1 year ago

@9il pushed new commit. Feel free to squash before merging.

9il commented 1 year ago

Please

make a single type API with ~~X = F = T~~ two types like in the Polynomial structure
unify the code in a single function
unify code with the Polynomial structure

jmh530 commented 1 year ago

@9il Addressed them in latest. First two are pretty simple. Last one you might check is how you want it (see: https://github.com/libmir/mir-algorithm/pull/453/commits/3d761cc1cc0052228c0241f5c2ac77fe0e1ba14a)

jmh530 commented 1 year ago

@9il That change results in the error below. It seems as if F.init for const(double)[] is null. I changed it to ForeachType!F.init and it worked. Though I'm happy to make some other adjustment instead.

source\mir\polynomial.d(100,12): Error: incompatible types for `(null) * (0)`: `const(double)[]` and `int`
source\mir\polynomial.d(44,37): Error: template instance `mir.polynomial.poly!0u.poly!(int, const(double)[])` error instantiating
source\mir\math\func\hermite.d(73,7):        instantiated from here: `opCall!int`
source\mir\polynomial.d(100,12): Error: incompatible types for `(null) * (nan)`: `const(double)[]` and `double`
dmd failed with exit code 1.

codecov-commenter commented 1 year ago

Codecov Report

Base: 91.56% // Head: 91.59% // Increases project coverage by +0.03% :tada:

Coverage data is based on head (f55bec8) compared to base (d20245f). Patch coverage: 100.00% of modified lines in pull request are covered.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #453 +/- ## ========================================== + Coverage 91.56% 91.59% +0.03% ========================================== Files 79 79 Lines 18739 18812 +73 ========================================== + Hits 17158 17231 +73 Misses 1581 1581 ``` | [Impacted Files](https://codecov.io/gh/libmir/mir-algorithm/pull/453?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=libmir) | Coverage Δ | | |---|---|---| | [source/mir/polynomial.d](https://codecov.io/gh/libmir/mir-algorithm/pull/453?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=libmir#diff-c291cmNlL21pci9wb2x5bm9taWFsLmQ=) | `98.70% <100.00%> (+2.14%)` | :arrow_up: | | [source/mir/math/sum.d](https://codecov.io/gh/libmir/mir-algorithm/pull/453?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=libmir#diff-c291cmNlL21pci9tYXRoL3N1bS5k) | `96.31% <0.00%> (+0.15%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=libmir). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=libmir)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

jmh530 commented 1 year ago

@9il Well it looks like my motivation for adding this is a bit mute as I was able to prod the phobos team to fix log1p. https://github.com/dlang/phobos/pull/8701

libmir / mir-algorithm

Add monic polynomial #453

Codecov Report