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

MathFluxExtensions Missing Methods, and Inconsistent #28

Closed
kiwisincebirth opened this issue Sep 29, 2021 · 5 comments · Fixed by #29
Closed

MathFluxExtensions Missing Methods, and Inconsistent #28

kiwisincebirth opened this issue Sep 29, 2021 · 5 comments · Fixed by #29
Labels
good first issue Ideal for a new contributor, we'll help status/need-decision This issue needs a decision type/enhancement A general enhancement

Comments

@kiwisincebirth
Copy link
Contributor

kiwisincebirth commented Sep 29, 2021

Motivation

The use of Kotlin Math Extensions is limited as it is missing several methods specific to sum() and average() of typed fluxes such as BigDecimal. I write financial application(s) which uses BigDecimal as a preference, using floats and doubles is discouraged, hence the need for Big Decimal specific implementations.

As such I have to implement these missing functions in my own Kotlin code, which defeats the purpose of this library. Traversing via Double is not an option.

Desired solution

Implement missing methods

The following is an example of a missing method, of which there are many examples.

fun <T> Flux<T>.sumBigDecimal(mapper: (T) -> Number): Mono<BigDecimal>
        = MathFlux.sumBigDecimal(this, Function(mapper)) 

Ideally there would be a one - to - one relationship between the Kotlin functions provided by this library and the functions they wrap in the core MathFlux implementation.

Untyped methods

In the library there are a few examples of untyped methods() e.g.

fun <T> Flux<T>.sum(mapper: (T) -> Number): Mono<Long>
        = MathFlux.sumLong(this, Function(mapper)) 

and

fun <T> Flux<T>.average(mapper: (T) -> Number): Mono<Double>
        = MathFlux.averageDouble(this, Function(mapper)) 

These untyped method make (opinions) on the underlying type that should be used. These opinions are also not consistent, since one method uses Long the other Double, unsure why this is.

Noting also that the underlying library MathFlux provides no such opinions, this is specific to the Kotlin Extension.

Could do one of the following.

(a) Deprecate the use of these untyped methods
(b) Make the untyped methods consistent e.g. Double (an opinion)
(c) Do Nothing. Leave the untyped methods as they are.

Considered alternatives

N/A

Additional context

PS. I am happy to submit a Pull Request if you like. Would need to understand the Scope of the change as discussed above, happy to take any direction on this.

PPS. This is a continuation of an issue I am trying to get resolved Part 1 was fixed in this issue.
reactor/reactor-addons#260

@reactorbot reactorbot added the ❓need-triage This issue needs triage, hasn't been looked at by a team member yet label Sep 29, 2021
@simonbasle
Copy link
Member

Missing methods

I can't remember why it wasn't done in the first place, other than limiting the maintenance burden by covering what seemed like the most used types at the type.

I think that's where effort should be focused.

Untyped methods

The goal of these methods was to provide an opinionated shortcut of mapping arbitrary types to numbers then summing/averaging these numbers in one go, without trying to cover all combinations.

IIRC the rationale was that a sum is more naturally expected to be an integer number while an average is more naturally expected to be a floating point number. I also wanted to avoid redundant xxxNumber suffixes in the extension methods where possible, because that looked a bit too verbose. The mere fact that I felt compelled to add sumDouble(mapper) indicates that first assumption on sums is a bit too brittle, though...

That said, if a user wants to map to a Long or Double for sums or to a Double for averages, then they have additional syntactic sugar. If not, then they can map anyway.

Providing Number-subclasses-specific extensions for sum/average would suffice to enable any combination, in that later case.

So if missing methods are fixed, is there really anything to be done for mapper-based extensions?

@simonbasle simonbasle added type/enhancement A general enhancement status/need-decision This issue needs a decision good first issue Ideal for a new contributor, we'll help and removed ❓need-triage This issue needs triage, hasn't been looked at by a team member yet labels Sep 29, 2021
@simonbasle
Copy link
Member

@kiwisincebirth please go ahead if you want to submit a PR for part 1

@kiwisincebirth
Copy link
Contributor Author

Pull Request Raised.

#29

@kiwisincebirth
Copy link
Contributor Author

IIRC the rationale was that a sum is more naturally expected to be an integer number while an average is more naturally expected to be a floating point number

My opinion on this is that a Double would be the best choice for a general purpose use for both sums and averages. Floating point numbers (while they have their issues), can represent any natural number, where as integers obviously do not support fractional numbers, without some form of introduced scaling.

if the library were to only support a single type of Number (within these functions), then Double would make the most sense to me. In my application (at least), the pain of converting a financial value to and from an integer to sum its value would be more painful, than accepting floating point number precision issues.

Just my 2 cents.

@kiwisincebirth
Copy link
Contributor Author

Off Topic: In the absence of sumBigDecimal() method in the library. Below is the function I am currently using. The below shows my opinionated view of how to use Kotlin to extend the capability of a framework in a natural and useful way.

fun <T> Flux<T>.sumBigDecimal(emptyZero: Boolean = false, mapper: (T) -> Number): Mono<BigDecimal> =
    if (emptyZero) 
        MathFlux.sumBigDecimal(this, Function(mapper)).switchIfEmpty( Mono.just(BigDecimal.ZERO) )
    else 
        MathFlux.sumBigDecimal(this, Function(mapper))

simonbasle pushed a commit that referenced this issue Oct 5, 2021
This commit introduces better extensions for summing and averaging,
which maintain the original exact Number type and precision, as
`sumAll()` and `averageAll()`.

A mapper-based version is also introduced, which also internally
ensures the correct MathFlux method is used (maintaining precision).

The old mapper-based version would internally systematically convert
to `Long` and end up creating a `Mono<Long>`, instead of the Number
type produced by the mapper. It has been deprecated.

Finally, a number of `sumAsXxx` and `averageAsXxx` methods have been
introduced which explicitly convert incoming Numbers to the Xxx type,
with possible loss of precision. These cover all the Number subtypes.

The sum/sumDouble/average extensions have been deprecated, and either
of sumAll/averageAll/sumAsXxx/averageAsXxx should be used instead.

Note that some `Since` annotations have also been corrected.

Fixes #28.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Ideal for a new contributor, we'll help status/need-decision This issue needs a decision type/enhancement A general enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants