WIP: feature/emd #521

Draft
teldufalsari wants to merge 11 commits from teldufalsari/kmath:feature/emd into dev
2 changed files with 68 additions and 67 deletions
Showing only changes of commit 4f2ebb17ff - Show all commits

View File

@ -6,11 +6,12 @@
package space.kscience.kmath.series package space.kscience.kmath.series
import space.kscience.kmath.structures.* import space.kscience.kmath.structures.*
import space.kscience.kmath.operations.invoke
import space.kscience.plotly.* import space.kscience.plotly.*
import space.kscience.plotly.models.Scatter import space.kscience.plotly.models.Scatter
import kotlin.math.sin import kotlin.math.sin
Review
fun main(): Unit = (Double.seriesAlgebra()) {

is enough if you import import space.kscience.kmath.operations.invoke.

``` fun main(): Unit = (Double.seriesAlgebra()) { ``` is enough if you import `import space.kscience.kmath.operations.invoke`.
fun main(): Unit = with(Double.seriesAlgebra()) { fun main(): Unit = (Double.seriesAlgebra()) {
val signal = DoubleArray(800) { val signal = DoubleArray(800) {
sin(it.toDouble() / 10.0) + 3.5 * sin(it.toDouble() / 60.0) sin(it.toDouble() / 10.0) + 3.5 * sin(it.toDouble() / 60.0)
}.asBuffer().moveTo(0) }.asBuffer().moveTo(0)

View File

@ -12,7 +12,6 @@ import space.kscience.kmath.operations.Float64BufferOps.Companion.div
import space.kscience.kmath.operations.Float64BufferOps.Companion.pow import space.kscience.kmath.operations.Float64BufferOps.Companion.pow
import space.kscience.kmath.structures.Buffer import space.kscience.kmath.structures.Buffer
import space.kscience.kmath.structures.last import space.kscience.kmath.structures.last
import space.kscience.kmath.structures.slice
import kotlin.math.sign import kotlin.math.sign
/** /**
@ -30,22 +29,23 @@ import kotlin.math.sign
* @param nModes how many modes should be extracted at most. The algorithm may return fewer modes if it was not * @param nModes how many modes should be extracted at most. The algorithm may return fewer modes if it was not
* possible to extract more modes from the signal. * possible to extract more modes from the signal.
*/ */
public class EmpiricalModeDecomposition<BA, L> ( public class EmpiricalModeDecomposition<BA, L: Number> (
private val seriesAlgebra: SeriesAlgebra<Double, *, BA, L>, private val seriesAlgebra: SeriesAlgebra<Double, *, BA, L>,
private val sConditionThreshold: Int = 15, private val sConditionThreshold: Int = 15,
private val maxSiftIterations: Int = 20, private val maxSiftIterations: Int = 20,
private val siftingDelta: Double = 1e-2, private val siftingDelta: Double = 1e-2,
private val nModes: Int = 6 private val nModes: Int = 6
) where BA: BufferAlgebra<Double, *>, BA: RingOps<Buffer<Double>>, L : Number { ) where BA: BufferAlgebra<Double, *>, BA: RingOps<Buffer<Double>> {
Review

Please, move the only type argument L's bound to L's declaration cite:

public class EmpiricalModeDecomposition<BA, L: Number> (
    private val seriesAlgebra: SeriesAlgebra<Double, *, BA, L>,
    private val sConditionThreshold: Int = 15,
    private val maxSiftIterations: Int = 20,
    private val siftingDelta: Double = 1e-2,
    private val nModes: Int = 6
) where BA: BufferAlgebra<Double, *>,  BA: RingOps<Buffer<Double>> {
Please, move the only type argument `L`'s bound to `L`'s declaration cite: ```kotlin public class EmpiricalModeDecomposition<BA, L: Number> ( private val seriesAlgebra: SeriesAlgebra<Double, *, BA, L>, private val sConditionThreshold: Int = 15, private val maxSiftIterations: Int = 20, private val siftingDelta: Double = 1e-2, private val nModes: Int = 6 ) where BA: BufferAlgebra<Double, *>, BA: RingOps<Buffer<Double>> { ```
Review

By the way, in general you'll have to use T as L. Otherwise, either interpolate can not be resolved in snippet

interpolator.interpolate(
    Buffer(extrema.size) { signal.labels[extrema[it]] },
    Buffer(extrema.size) { signal[extrema[it]] }
)

or there is no function to map L values to T.

By the way, in general you'll have to use `T` as `L`. Otherwise, either `interpolate` can not be resolved in snippet ```kotlin interpolator.interpolate( Buffer(extrema.size) { signal.labels[extrema[it]] }, Buffer(extrema.size) { signal[extrema[it]] } ) ``` or there is no function to map `L` values to `T`.
/** /**
* Take a signal, construct an upper and a lower envelope, find the mean value of two, * Take a signal, construct an upper and a lower envelopes, find the mean value of two,
* represented as a series. * represented as a series.

Possible typos fix suggestion:

     * Take a signal, construct an upper and a lower envelopes, find the mean value of the two,
Possible typos fix suggestion: ``` * Take a signal, construct an upper and a lower envelopes, find the mean value of the two, ```
* @param signal Signal to compute on * @param signal Signal to compute on
* *
* @return null is returned if the signal has not enough extrema to construct envelopes. * @return mean [Series] or `null`. `null` is returned in case
* the signal does not have enough extrema to construct envelopes.

I suggest adding a sentence that describes what is actually returned. And possible typos fix as well.

     * @return The mean series or `null`. `null` is returned if the signal does not have enough extrema to construct envelopes.
I suggest adding a sentence that describes what is actually returned. And possible typos fix as well. ``` * @return The mean series or `null`. `null` is returned if the signal does not have enough extrema to construct envelopes. ```
*/ */
private fun findMean(signal: Series<Double>): Series<Double>? = with(seriesAlgebra) { private fun findMean(signal: Series<Double>): Series<Double>? = (seriesAlgebra) {
    private fun findMean(signal: Series<Double>): Series<Double>? = seriesAlgebra {

Just this is enough. Because there is invoke extension operator implemented that is imported here.

```kotlin private fun findMean(signal: Series<Double>): Series<Double>? = seriesAlgebra { ``` Just this is enough. Because there is `invoke` extension operator implemented that is imported here.
val interpolator = SplineInterpolator(Float64Field) val interpolator = SplineInterpolator(Float64Field)
fun generateEnvelope(extrema: List<Int>): Series<Double> { fun generateEnvelope(extrema: List<Int>): Series<Double> {
val envelopeFunction = interpolator.interpolate( val envelopeFunction = interpolator.interpolate(
@ -59,12 +59,12 @@ public class EmpiricalModeDecomposition<BA, L> (
// need to make the interpolator yield values outside boundaries? // need to make the interpolator yield values outside boundaries?
} }
} }
val maxima = signal.maxima() val maxima = signal.paddedMaxima()
val minima = signal.minima() val minima = signal.paddedMinima()
return if (maxima.size < 3 || minima.size < 3) null else { return if (maxima.size < 3 || minima.size < 3) null else {
val upperEnvelope = generateEnvelope(maxima) val upperEnvelope = generateEnvelope(maxima)
val lowerEnvelope = generateEnvelope(minima) val lowerEnvelope = generateEnvelope(minima)
return (upperEnvelope + lowerEnvelope).map { it * 0.5 } return upperEnvelope.zip(lowerEnvelope) { upper, lower -> upper + lower / 2.0 }
            return upperEnvelope.zip(lowerEnvelope) { left, right -> (left + right) / 2 }
```kotlin return upperEnvelope.zip(lowerEnvelope) { left, right -> (left + right) / 2 } ```
} }
} }
@ -76,15 +76,16 @@ public class EmpiricalModeDecomposition<BA, L> (
* @return [SiftingResult.NotEnoughExtrema] is returned if the signal has too few extrema to extract a mode. * @return [SiftingResult.NotEnoughExtrema] is returned if the signal has too few extrema to extract a mode.
* Success of an appropriate type (See [SiftingResult.Success] class) is returned otherwise. * Success of an appropriate type (See [SiftingResult.Success] class) is returned otherwise.
*/ */
private fun sift(signal: Series<Double>): SiftingResult = with(seriesAlgebra) { private fun sift(signal: Series<Double>): SiftingResult = (seriesAlgebra) {

As well as I understand, whole body of the function can be replaced with just

    private fun sift(signal: Series<Double>): SiftingResult = siftInner(signal, 1, 0)
As well as I understand, whole body of the function can be replaced with just ```kotlin private fun sift(signal: Series<Double>): SiftingResult = siftInner(signal, 1, 0) ```

Also siftInner should be marked tailrec, shouldn't it?

Also `siftInner` should be marked `tailrec`, shouldn't it?

Yeah, it is better if the function is marked tailrec. But I am not sure if compiler understands the case. So I need a bit of time for a small test.

Yeah, it is better if the function is marked `tailrec`. But I am not sure if compiler understands the case. So I need a bit of time for a small test.

It works. With tailrec it does not produce stack overflow when I run sifting with 5000 iterations per mode

It works. With `tailrec` it does not produce stack overflow when I run sifting with 5000 iterations per mode
val mean = findMean(signal) ?: return SiftingResult.NotEnoughExtrema val mean = findMean(signal) ?: return SiftingResult.NotEnoughExtrema
val protoMode = signal.zip(mean) { s, m -> s - m } val protoMode = signal.zip(mean) { s, m -> s - m }
val sNumber = if (protoMode.sCondition()) 1 else 0 val sNumber = if (protoMode.sCondition()) 1 else 0
return if (maxSiftIterations == 1) SiftingResult.MaxIterationsReached(protoMode) return when {
else if (sNumber >= sConditionThreshold) SiftingResult.SNumberReached(protoMode) maxSiftIterations == 1 -> SiftingResult.MaxIterationsReached(protoMode)
else if (relativeDifference(signal, protoMode) < siftingDelta * signal.size) { sNumber >= sConditionThreshold -> SiftingResult.SNumberReached(protoMode)
SiftingResult.DeltaReached(protoMode) relativeDifference(protoMode, signal) < siftingDelta * signal.size -> SiftingResult.DeltaReached(protoMode)
} else siftInner(protoMode, 2, sNumber) else -> siftInner(protoMode, 2, sNumber)
}
} }
/** /**
@ -94,14 +95,16 @@ public class EmpiricalModeDecomposition<BA, L> (
prevMode: Series<Double>, prevMode: Series<Double>,
iterationNumber: Int, iterationNumber: Int,
sNumber: Int sNumber: Int
): SiftingResult = with(seriesAlgebra) { ): SiftingResult = (seriesAlgebra) {
val mean = findMean(prevMode) ?: return SiftingResult.SignalFlattened(prevMode) val mean = findMean(prevMode) ?: return SiftingResult.SignalFlattened(prevMode)
val mode = prevMode.zip(mean) { p, m -> p - m } val mode = prevMode.zip(mean) { p, m -> p - m }
val newSNumber = if (mode.sCondition()) sNumber + 1 else sNumber val newSNumber = if (mode.sCondition()) sNumber + 1 else sNumber
Review

Gosh. Use when instead of long if-else sequence, please:

        return when {
            iterationNumber >= maxSiftIterations -> SiftingResult.MaxIterationsReached(mode)
            sNumber >= sConditionThreshold -> SiftingResult.SNumberReached(mode)
            relativeDifference(prevMode, mode) < siftingDelta * mode.size -> SiftingResult.DeltaReached(mode)
            else -> siftInner(mode, iterationNumber + 1, newSNumber)
        }

It's idiom, and it's clearer to read.

Gosh. Use `when` instead of long if-else sequence, please: ```kotlin return when { iterationNumber >= maxSiftIterations -> SiftingResult.MaxIterationsReached(mode) sNumber >= sConditionThreshold -> SiftingResult.SNumberReached(mode) relativeDifference(prevMode, mode) < siftingDelta * mode.size -> SiftingResult.DeltaReached(mode) else -> siftInner(mode, iterationNumber + 1, newSNumber) } ``` It's idiom, and it's clearer to read.
return if (iterationNumber >= maxSiftIterations) SiftingResult.MaxIterationsReached(mode) return when {
else if (sNumber >= sConditionThreshold) SiftingResult.SNumberReached(mode) iterationNumber >= maxSiftIterations -> SiftingResult.MaxIterationsReached(mode)

Is it intended that previous mode is assigned to current parameter of relativeDifference and current (new) mode is assigned to previous parameter of relativeDifference? The parameters names say that there is a swap of parameters.

I would use explicit parameters' assignation:

relativeDifference(previous = prevMode, current = mode)
Is it intended that previous mode is assigned to `current` parameter of `relativeDifference` and current (new) mode is assigned to `previous` parameter of `relativeDifference`? The parameters names say that there is a swap of parameters. I would use explicit parameters' assignation: ```kotlin relativeDifference(previous = prevMode, current = mode) ```
else if (relativeDifference(prevMode, mode) < siftingDelta * mode.size) SiftingResult.DeltaReached(mode) sNumber >= sConditionThreshold -> SiftingResult.SNumberReached(mode)
else siftInner(mode, iterationNumber + 1, newSNumber) relativeDifference(mode, prevMode) < siftingDelta * mode.size -> SiftingResult.DeltaReached(mode)
else -> siftInner(mode, iterationNumber + 1, newSNumber)
}
} }
/** /**
@ -112,20 +115,17 @@ public class EmpiricalModeDecomposition<BA, L> (
* Modes returned in a list which contains as many modes as it was possible * Modes returned in a list which contains as many modes as it was possible

The whole function can be rewritten in such way:

    public fun decompose(signal: Series<Double>): EMDecompositionResult = with(seriesAlgebra) {
        val modes = mutableListOf<Series<Double>>()
        var residual = signal
        repeat(nModes) {
            val nextMode = when(val r = sift(residual)) {
                SiftingResult.NotEnoughExtrema ->
                    return EMDecompositionResult(
                        if (it == 0) EMDTerminationReason.SIGNAL_TOO_FLAT
                        else EMDTerminationReason.ALL_POSSIBLE_MODES_EXTRACTED,
                        modes
                    )
                is SiftingResult.Success -> r.result
            }
            modes.add(nextMode)
            residual = residual.zip(nextMode) { l, r -> l - r }
        }
        return EMDecompositionResult(EMDTerminationReason.MAX_MODES_REACHED, modes)
    }

It's shorter but as readable as the previous version.

The whole function can be rewritten in such way: ```kotlin public fun decompose(signal: Series<Double>): EMDecompositionResult = with(seriesAlgebra) { val modes = mutableListOf<Series<Double>>() var residual = signal repeat(nModes) { val nextMode = when(val r = sift(residual)) { SiftingResult.NotEnoughExtrema -> return EMDecompositionResult( if (it == 0) EMDTerminationReason.SIGNAL_TOO_FLAT else EMDTerminationReason.ALL_POSSIBLE_MODES_EXTRACTED, modes ) is SiftingResult.Success -> r.result } modes.add(nextMode) residual = residual.zip(nextMode) { l, r -> l - r } } return EMDecompositionResult(EMDTerminationReason.MAX_MODES_REACHED, modes) } ``` It's shorter but as readable as the previous version.
* to extract before triggering one of the termination conditions. * to extract before triggering one of the termination conditions.
*/ */
public fun decompose(signal: Series<Double>): EMDecompositionResult = with(seriesAlgebra) { public fun decompose(signal: Series<Double>): EMDecompositionResult = (seriesAlgebra) {
val modes = mutableListOf<Series<Double>>() val modes = mutableListOf<Series<Double>>()
val mode = when(val r = sift(signal)) { var residual = signal
SiftingResult.NotEnoughExtrema -> repeat(nModes) {
return EMDecompositionResult(EMDTerminationReason.SIGNAL_TOO_FLAT, modes)
is SiftingResult.Success -> r.result
}
modes.add(mode)
var residual = signal.zip(mode) { l, r -> l - r }
repeat(nModes - 1) {
val nextMode = when(val r = sift(residual)) { val nextMode = when(val r = sift(residual)) {
SiftingResult.NotEnoughExtrema -> SiftingResult.NotEnoughExtrema ->
return EMDecompositionResult(EMDTerminationReason.ALL_POSSIBLE_MODES_EXTRACTED, modes) return EMDecompositionResult(

You should use SeriesAlgebra's elementAlgebra in getting difference of two Double values instead of l - r. And in 8 strings below too.

You should use `SeriesAlgebra`'s `elementAlgebra` in getting difference of two `Double` values instead of `l - r`. And in 8 strings below too.
if (it == 0) EMDTerminationReason.SIGNAL_TOO_FLAT
else EMDTerminationReason.ALL_POSSIBLE_MODES_EXTRACTED,
modes
)
is SiftingResult.Success -> r.result is SiftingResult.Success -> r.result
} }
modes.add(nextMode) modes.add(nextMode)
@ -149,13 +149,13 @@ public class EmpiricalModeDecomposition<BA, L> (
* @param nModes how many modes should be extracted at most. The algorithm may return fewer modes if it was not * @param nModes how many modes should be extracted at most. The algorithm may return fewer modes if it was not
* possible to extract more modes from the signal. * possible to extract more modes from the signal.
*/ */
public fun <L, BA> SeriesAlgebra<Double, *, BA, L>.empiricalModeDecomposition( public fun <L: Number, BA> SeriesAlgebra<Double, *, BA, L>.empiricalModeDecomposition(
sConditionThreshold: Int = 15, sConditionThreshold: Int = 15,
maxSiftIterations: Int = 20, maxSiftIterations: Int = 20,
siftingDelta: Double = 1e-2, siftingDelta: Double = 1e-2,
nModes: Int = 3 nModes: Int = 3
): EmpiricalModeDecomposition<BA, L> ): EmpiricalModeDecomposition<BA, L>
where BA: BufferAlgebra<Double, *>, BA: RingOps<Buffer<Double>>, L: Number = EmpiricalModeDecomposition( where BA: BufferAlgebra<Double, *>, BA: RingOps<Buffer<Double>> = EmpiricalModeDecomposition(
seriesAlgebra = this, seriesAlgebra = this,
sConditionThreshold = sConditionThreshold, sConditionThreshold = sConditionThreshold,
maxSiftIterations = maxSiftIterations, maxSiftIterations = maxSiftIterations,
@ -168,22 +168,25 @@ where BA: BufferAlgebra<Double, *>, BA: RingOps<Buffer<Double>>, L: Number = Emp
*/ */
private fun Series<Double>.countZeros(): Int { private fun Series<Double>.countZeros(): Int {
require(size >= 2) { "Expected series with at least 2 elements, but got $size elements" } require(size >= 2) { "Expected series with at least 2 elements, but got $size elements" }
return fold(Pair(get(0), 0)) { acc: Pair<Double, Int>, it: Double -> data class SignCounter(val prevSign: Double, val zeroCount: Int)

Do not use Pair! It's non-idiomatic in Kotlin. It is really hard to always keep in mind what first and second fields hold. Instead, define and use custom data class with understandable name and understandable parameters' names.

Do not use `Pair`! It's non-idiomatic in Kotlin. It is really hard to always keep in mind what `first` and `second` fields hold. Instead, define and use custom data class with understandable name and understandable parameters' names.

Is making a one-line data class declaration above considered idiomatic?

    data class SignCounter(val prevSign: Double, val zeroCount: Int)
    return fold(SignCounter(sign(get(0)), 0)) { acc: SignCounter, it: Double ->
Is making a one-line data class declaration above considered idiomatic? ```kotlin data class SignCounter(val prevSign: Double, val zeroCount: Int) return fold(SignCounter(sign(get(0)), 0)) { acc: SignCounter, it: Double -> ```

Yeah, that's the idiomatic way. But place it outside the function. Otherwise, you won't able to access it :)

Yeah, that's the idiomatic way. But place it outside the function. Otherwise, you won't able to access it :)
if (sign(acc.first) != sign(it)) Pair(it, acc.second + 1) else Pair(it, acc.second)

It's better to store sign instead of the value which sign is compared, to not compute it each iteration.

It's better to store sign instead of the value which sign is compared, to not compute it each iteration.
}.second return fold(SignCounter(sign(get(0)), 0)) { acc: SignCounter, it: Double ->
val currentSign = sign(it)
if (acc.prevSign != currentSign) SignCounter(currentSign, acc.zeroCount + 1)
else SignCounter(currentSign, acc.zeroCount)
}.zeroCount
} }
/** /**
* Compute relative difference of two series. * Compute relative difference of two series.
*/ */

Use = notation for inline bodies, please:

private fun <A: Ring<Double>, BA> SeriesAlgebra<Double, A, BA, *>.relativeDifference(
    current: Series<Double>,
    previous: Series<Double>
):Double where BA: BufferAlgebra<Double, A>, BA: RingOps<Buffer<Double>> =
    (current - previous).pow(2)
        .div(previous pow 2)
        .fold(0.0) { acc, d -> elementAlgebra.add(acc, d) }

Also:

  1. L is not used, so I removed it.
  2. Default algebra used when SeriesAlgebra's elementAlgebra is needed. So I replaced it. It will also help with refactoring from Double "algebras" to general algebras.
  3. No, fold uses Double as a type parameter, so boxing is not avoided. I would replace .fold(0.0) { acc, d -> acc + d } with .sum(elementAlgebra) but there is no such operation for some reason.
Use `=` notation for inline bodies, please: ```kotlin private fun <A: Ring<Double>, BA> SeriesAlgebra<Double, A, BA, *>.relativeDifference( current: Series<Double>, previous: Series<Double> ):Double where BA: BufferAlgebra<Double, A>, BA: RingOps<Buffer<Double>> = (current - previous).pow(2) .div(previous pow 2) .fold(0.0) { acc, d -> elementAlgebra.add(acc, d) } ``` Also: 1. `L` is not used, so I removed it. 2. Default algebra used when `SeriesAlgebra`'s `elementAlgebra` is needed. So I replaced it. It will also help with refactoring from `Double` "algebras" to general algebras. 3. No, `fold` uses `Double` as a type parameter, so boxing is not avoided. I would replace `.fold(0.0) { acc, d -> acc + d }` with `.sum(elementAlgebra)` but there is no such operation for some reason.

I also wondered why there is no .sum() method. I could implement it for a 1-d series, but doing it for a general buffer seems a bit too much if there is need for arbitrary axis like in NumPy

I also wondered why there is no `.sum()` method. I could implement it for a 1-d series, but doing it for a general buffer seems a bit too much if there is need for arbitrary axis like in NumPy

@altavir There is a need for a function Buffer<T>.sum(elementAlgebra: Group<T>). Where should we place it?

@altavir There is a need for a function `Buffer<T>.sum(elementAlgebra: Group<T>)`. Where should we place it?
private fun <L, BA> SeriesAlgebra<Double, *, BA, L>.relativeDifference( private fun <BA> SeriesAlgebra<Double, *, BA, *>.relativeDifference(
current: Series<Double>, current: Series<Double>,
previous: Series<Double> previous: Series<Double>
):Double where BA: BufferAlgebra<Double, *>, BA: RingOps<Buffer<Double>>, L: Number { ):Double where BA: BufferAlgebra<Double, *>, BA: RingOps<Buffer<Double>> =
return (current - previous).pow(2) (current - previous).pow(2)
.div(previous pow 2) .div(previous pow 2)
.fold(0.0) { acc, d -> acc + d } // to avoid unnecessary boxing, but i may be wrong .fold(0.0) { acc, d -> acc + d } // TODO replace with Series<>.sum() method when it's implemented
}
private fun <T: Comparable<T>> isExtreme(prev: T, elem: T, next: T): Boolean = private fun <T: Comparable<T>> isExtreme(prev: T, elem: T, next: T): Boolean =
(elem > prev && elem > next) || (elem < prev && elem < next) (elem > prev && elem > next) || (elem < prev && elem < next)
@ -193,43 +196,40 @@ private fun <T: Comparable<T>> isExtreme(prev: T, elem: T, next: T): Boolean =
*/ */

I would recommend writing

    return (1 .. size - 2).count { isExtreme(this[it-1], this[it], this[it+1]) }
I would recommend writing ```kotlin return (1 .. size - 2).count { isExtreme(this[it-1], this[it], this[it+1]) } ```
private fun Series<Double>.countExtrema(): Int { private fun Series<Double>.countExtrema(): Int {
require(size >= 3) { "Expected series with at least 3 elements, but got $size elements" } require(size >= 3) { "Expected series with at least 3 elements, but got $size elements" }
return slice(1..< size - 1).asIterable().foldIndexed(0) { index, acc, it -> return (1 .. size - 2).count { isExtreme(this[it - 1], this[it], this[it + 1]) }
if (isExtreme(get(index), it, get(index + 2))) acc + 1 else acc
}
} }
/**
* Retrieve indices of knot points for spline interpolation matching the predicate.
* The first and the last points of a series are always included.
*/
private fun <T : Comparable<T>> Series<T>.maxima(): List<Int> {
```kotlin private fun <T : Comparable<T>> Series<T>.maxima(): List<Int> { ```
private fun <T: Comparable<T>> Series<T>.knotPoints(predicate: (T, T, T) -> Boolean): List<Int> {
require(size >= 3) { "Expected series with at least 3 elements, but got $size elements" }
val points = mutableListOf(0)

I would recommend rewriting it with old good plain loop on indices:

    for (index in 1 .. size - 2) {
        val left = this[index-1]
        val middle = this[index]
        val right = this[index+1]
        if (middle > left && middle > right) maxima.add(index)
    }

or using

    return (1 .. size - 2).filter { (this[it] > this[it-1] && this[it] > this[it+1]) || it == 0 || it == size - 1 }
I would recommend rewriting it with old good plain loop on indices: ```kotlin for (index in 1 .. size - 2) { val left = this[index-1] val middle = this[index] val right = this[index+1] if (middle > left && middle > right) maxima.add(index) } ``` or using ```kotlin return (1 .. size - 2).filter { (this[it] > this[it-1] && this[it] > this[it+1]) || it == 0 || it == size - 1 } ```

Also, is it intended that the spline will ignore double extrema?

I mean for series [0.0, -1.0, -1.0, 1.0, 1.0, -1.0, -1.0, 0.0] there will be no maxima and minima points but the end points.

Also, is it intended that the spline will ignore double extrema? I mean for series `[0.0, -1.0, -1.0, 1.0, 1.0, -1.0, -1.0, 0.0]` there will be no maxima and minima points but the end points.

Also, is it intended that the spline will ignore double extrema?

No, I'm planning on improving this function and making it public placed in SeriesExtensions.kt

> Also, is it intended that the spline will ignore double extrema? No, I'm planning on improving this function and making it `public` placed in `SeriesExtensions.kt`
for (index in 1 .. size - 2) {

Could you comment what question // weird offset, is there a way to do it better? means in more detail?

Could you comment what question ` // weird offset, is there a way to do it better?` means in more detail?
val left = this[index - 1]
val middle = this[index]
val right = this[index + 1]
if (predicate(left, middle, right)) points.add(index)
}
points.add(size - 1)
return points
}
/** /**
* Retrieve indices of knot points used to construct an upper envelope, * Retrieve indices of knot points used to construct an upper envelope,
* namely maxima together with the first last point in a series. * namely maxima together with the first last point in a series.
private fun <T : Comparable<T>> Series<T>.minima(): List<Int> {
```kotlin private fun <T : Comparable<T>> Series<T>.minima(): List<Int> { ```
*/ */
private fun<T> Series<T>.maxima(): List<Int> where T: Comparable<T> { private fun <T: Comparable<T>> Series<T>.paddedMaxima(): List<Int> =
require(size >= 3) { "Expected series with at least 3 elements, but got $size elements" } knotPoints { left, middle, right -> (middle > left && middle > right) }

I would recommend rewriting it with old good plain loop on indices:

    for (index in 1 .. size - 2) {
        val left = this[index-1]
        val middle = this[index]
        val right = this[index+1]
        if (middle < left && middle < right) maxima.add(index)
    }

or using

    return (1 .. size - 2).filter { (this[it] < this[it-1] && this[it] < this[it+1]) || it == 0 || it == size - 1 }
I would recommend rewriting it with old good plain loop on indices: ```kotlin for (index in 1 .. size - 2) { val left = this[index-1] val middle = this[index] val right = this[index+1] if (middle < left && middle < right) maxima.add(index) } ``` or using ```kotlin return (1 .. size - 2).filter { (this[it] < this[it-1] && this[it] < this[it+1]) || it == 0 || it == size - 1 } ```

Also, similar question to this one.

Also, similar question to [this one](https://git.sciprog.center/kscience/kmath/pulls/521/files#issuecomment-1868).
val maxima = mutableListOf(0)
slice(1..< size - 1).asIterable().forEachIndexed { index, it ->
if (it > get(index) && it > get(index + 2)) { // weird offset, is there a way to do it better?
maxima.add(index + 1)
}
}
maxima.add(size - 1)
return maxima
}
/** /**
* Retrieve indices of knot points used to construct a lower envelope, * Retrieve indices of knot points used to construct a lower envelope,
* namely minima together with the first last point in a series. * namely minima together with the first last point in a series.
*/ */
private fun<T> Series<T>.minima(): List<Int> where T: Comparable<T> { private fun <T: Comparable<T>> Series<T>.paddedMinima(): List<Int> =
require(size >= 3) { "Expected series with at least 3 elements, but got $size elements" } knotPoints { left, middle, right -> (middle < left && middle < right) }
val minima = mutableListOf(0)
slice(1..< size - 1).asIterable().forEachIndexed { index, it ->
if (it < get(index) && it < get(index + 2)) { // weird offset, is there a way to do it better?
minima.add(index + 1)
}
}
minima.add(size - 1)
return minima
}
/** /**
* Check whether the numbers of zeroes and extrema of a series differ by no more than 1. * Check whether the numbers of zeroes and extrema of a series differ by no more than 1.