MHKiT-Software / MHKiT-Python

MHKiT-Python provides the marine renewable energy (MRE) community tools for data processing, visualization, quality control, resource assessment, and device performance.
https://mhkit-software.github.io/MHKiT/
BSD 3-Clause "New" or "Revised" License
47 stars 45 forks source link

Ensure interpolation values are within range when sampling contours #278

Closed ssolson closed 7 months ago

ssolson commented 7 months ago

Overview

In #218 @Graham-EGI uncovered a bug in mhkit.wave.contours.samples_contour

What he uncovered was quite simple:

Reference Example from #218

from mhkit.wave.contours import samples_contour
import numpy as np

contour = {
        "hm_contour": [ <see #218 for values> ],
        ""period_contour": [ <see #218 for values> ],
]

min_tp = min(contour["period_contour"])
max_tp = max(contour["period_contour"])

# ten samples
period_samples = np.linspace(min_tp, max_tp, 10)

# error thrown about being above the interpolation range
hm_samples = samples_contour(period_samples, np.array(contour["period_contour"]), np.array(contour["hm_contour"]))

The import part to note in the code block that period_samples is defined using max_tp.

The reason this error is thrown is because the sampling is performed by breaking the passed contour into 2 halves. The upper and lower half called w1 and w2 in samples_contour.

image

samples_contour chooses the upper half of the contour: image

As can be seen in the second plot the upper half ("chosen segment # 2") is not inclusive of the Max Tp value (red circle on right side). This max Tp value was used as the max of the Tp sample range. Therefore the interpolation throws an error as the sample is outside the data included for interpolation.

Solution

To fix this:

  1. The PR adds a check to ensure all samples are within the provided data range.
  2. After choosing the upper half of the contour the PR will add 1 index value to the chosen half if the min/max sample value is outside the range of the chosen half.
ssolson commented 7 months ago

@cmichelenstrofer could you give me a quick sanity check on the approach I took to fix this bug?

ssolson commented 7 months ago

@akeeste I am 99% certain what I did is correct and was mostly just checking with Carlos just in case. If you want to give a formal review while we wait I believe this PR is ready.

cmichelenstrofer commented 7 months ago

Caveat: I didn't run it to make sure it works.

But the logic & implementation look good to me. If I understand correctly these are the steps in the code:

based on how the data is divided the max/minimum if missing should only be one index away.