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

Wave annual average energy matrix graphic bug #204

Closed alimayt closed 1 year ago

alimayt commented 1 year ago

Hello. I found an error in the calculation performed in the graphics.py for plot_avg_annual_energy_matrix. The code should be taking an average (I believe), but instead it is a loop that iteratively adds the next value to the previous value and divides by 2, meaning (for example), for 3 years the loop would give an output of x0/4 + x1/4 +x2/2 instead of x0/3+ x1/3 + x2/3.

Beginning at line 455, it reads,

Initialize average annually with first year

avg_annual_counts_hist = hist_counts[years[0]]
avg_annual_J_hist = hist_J[years[0]]

# Iterate over each year and average
for year in years[1:]:
    avg_annual_counts_hist = (avg_annual_counts_hist+ hist_counts[year])/2
    avg_annual_J_hist = (avg_annual_J_hist+ hist_J[year])/2

When I think it should read:

Initialize average annually with first year

avg_annual_counts_hist = hist_counts[years[0]]/len(years)
avg_annual_J_hist = hist_J[years[0]]/len(years)

# Iterate over each year and average
for year in years[1:]:
    avg_annual_counts_hist = avg_annual_counts_hist+ hist_counts[year]/len(years)
    avg_annual_J_hist = avg_annual_J_hist+ hist_J[year]/len(years)
cmichelenstrofer commented 1 year ago

current:

# Initialize average annually with first year
avg_annual_counts_hist = hist_counts[years[0]]
avg_annual_J_hist = hist_J[years[0]]

# Iterate over each year and average
for year in years[1:]:
    avg_annual_counts_hist = (avg_annual_counts_hist+ hist_counts[year])/2
    avg_annual_J_hist = (avg_annual_J_hist+ hist_J[year])/2

Proposed:

# Initialize average annually with first year
avg_annual_counts_hist = hist_counts[years[0]]/len(years)
avg_annual_J_hist = hist_J[years[0]]/len(years)

# Iterate over each year and average
for year in years[1:]:
    avg_annual_counts_hist = avg_annual_counts_hist+ hist_counts[year]/len(years)
    avg_annual_J_hist = avg_annual_J_hist+ hist_J[year]/len(years)
cmichelenstrofer commented 1 year ago

@ssolson @rpauly18 you wrote this function originally, can you confirm it is a bug?

ssolson commented 1 year ago

hmm potentially. I didn't write this (I probably approved it though) but I believe this method is in the standard. I can check the standard and what exactly we are doing here this week.

ssolson commented 1 year ago

Thank you @alimayt for bringing this to my attention. I agree this is a bug and to correct my earlier statement I think I did write this. I propose the following:

Current:

# Initialize average annually with first year
avg_annual_counts_hist = hist_counts[years[0]]
avg_annual_J_hist = hist_J[years[0]]

# Iterate over each year and average
for year in years[1:]:
    avg_annual_counts_hist = (avg_annual_counts_hist+ hist_counts[year])/2
    avg_annual_J_hist = (avg_annual_J_hist+ hist_J[year])/2

which produces the following plot in PacWave_resource_characterization_example.ipynb image

Proposed:

avg_annual_counts_hist = sum(hist_counts.values())/len(years)
avg_annual_J_hist = sum(hist_J.values())/len(years)

Which produces the following plot (I additionally added a border to make the bin counts distinct from the contour) image

I will create a PR this week to address these changes.