[olivia-bentley]

Summary

Fixes #494. maximise_data=True had no effect in Shear.TimeSeries. Shear is now calculated for every timestamp that has at least two valid wind speeds (above min_speed and not NaN), as the docstring says. At each timestamp it uses only the heights that are valid.

Problem

When Shear.TimeSeries was initialised, a timestamp was only used if every height was valid ((wspds > min_speed).all(axis=1)), whatever maximise_data was set to. So maximise_data=True gave the same result as False. Timestamps where one anemometer was missing or below min_speed were thrown away, even when two or more good heights were left to fit the shear profile to.

Changes

• brightwind/analyse/shear.py
  • Shear.TimeSeries.__init__:
    • Now builds a per-height valid_heights mask.
    • With maximise_data=True: uses every timestamp with ≥ 2 valid heights and fits only the valid heights in each row.
    • With maximise_data=False: unchanged, still needs all heights to be valid.
  • Shear._calc_linear_fit:
    • Takes a new optional mask argument, so a vectorised least squares fit can use a different set of heights in each row.
    • Rows with fewer than two distinct heights return NaN for slope and intercept.
    • With no mask, the original code path is used.
• CHANGELOG.md: added a Bug Fixes entry under 2.8.0.
• tests/test_shear.py
  • test_calc_linear_fit:
    • masked fits match np.polyfit on the valid values of each row
    • a full mask matches the no-mask result
    • rows with fewer than two distinct heights give NaN
  • New test_time_series_maximise_data, run for power_law and log_law with NaNs and below-min_speed values injected:
    • timestamps where every height is valid are unchanged
    • timestamps with fewer than two valid heights are NaN
    • timestamps with exactly two valid heights match the analytical two-point power law / log law solution
    • the coverage in info['output data']['concurrent_period'] goes up (0.941 → 1.455)
  • test_time_series: the maximise_data=True objects were being overwritten and never checked. They now get their own variables and assertions.

Behaviour change

• With maximise_data=False (the default), results are unchanged.
• With maximise_data=True, results are unchanged for timestamps where every height is valid. Timestamps that were NaN before now get a shear value if at least two heights are valid.
• Anyone who has been using maximise_data=True will get more non-NaN values and a different mean or distribution of alpha or roughness, because two-height fits are now included.

Notes

• This branch is built on top of iss408_speed_up_wind_shear_calculation, which adds the vectorised _calc_linear_fit.

[BiancaMorandi]

@olivia-bentley I have reviewed this PR and the bug is solved. I see however some other issues below linked to maximise_data.

Gaps

1. Validation ignores maximise_data. _valid_wsp_data_error_msg raises "no valid data above min_speed" unless at least one timestamp has every height valid. So a dataset where the heights never all overlap raises an error, even though maximise_data=True could calculate shear on pairs. It also raises if any single column is entirely NaN.
2. Plot doesn't follow maximise_data. TimeSeries builds its plot via Shear.Average(wspds=...) (shear.py:107) without passing maximise_data. The profile plot therefore uses only timestamps with every height valid, while alpha/roughness use the larger dataset.
3. Dead code in _calc_power_law / _calc_log_law (shear.py:796, 829):
   • Nothing calls them with maximise_data=True. Average, BySector and TimeOfDay all use the default.
   • That branch also only drops speeds equal to 0, not NaN or values below min_speed, so it would give wrong results if anything did call it.
   • Worth removing the parameter, or fixing it if you want to add maximise_data to the other classes later.
4. info doesn't record the setting. _create_info doesn't store maximise_data, so you can't tell from info which mode was used. concurrent_period just changes meaning.

Suggested next steps

1. Make the validation check respect maximise_data: require at least two valid heights at some timestamp when it's True. I suggest to address as part of this PR.
2. store on create_info themaximise_data` input. I suggest to address as part of this PR.
3. Pass the mode through to the plot, or document that the plot always uses only timestamps with every height valid. Should this be addressed as part of bw.Shear.Timeseries - Timeseries calculates Shear twice and generates plot based on Average method #341 ?
4. Remove the unused maximise_data branches in _calc_power_law/_calc_log_law or add a separate enhancement issue to use maximise_data functionality of _calc_power_law/_calc_log_law also on Average, BySector and TimeOfDay. One or the other option should be just tracked on a issue for future work as this is very low priority to address. It is just good to have it on the brightwind issue list for future work. I leave @stephenholleran decide if he prefers to remove the unused maximise_data or enhance Average, BySector and TimeOfDay to use this option.

@stephenholleran when point 1 and 2 on 'suggested next steps' are addressed then I am happy for you to approve.

[olivia-bentley]

@olivia-bentley I have reviewed this PR and the bug is solved. I see however some other issues below linked to maximise_data.

Gaps

1. Validation ignores `maximise_data`. `_valid_wsp_data_error_msg `raises "no valid data above min_speed" unless at least one timestamp has every height valid. So a dataset where the heights never all overlap raises an error, even though `maximise_data`=True could calculate shear on pairs. It also raises if any single column is entirely NaN.

2. Plot doesn't follow `maximise_data`. `TimeSeries `builds its plot via `Shear.Average(wspds=...) `(shear.py:107) without passing `maximise_data`. The profile plot therefore uses only timestamps with every height valid, while alpha/roughness use the larger dataset.

3. Dead code in `_calc_power_law `/ `_calc_log_law `(shear.py:796, 829):
   
   * Nothing calls them with` maximise_data=True`. `Average`, `BySector `and `TimeOfDay `all use the default.
   * That branch also only drops speeds equal to 0, not NaN or values below `min_speed`, so it would give wrong results if anything did call it.
   * Worth removing the parameter, or fixing it if you want to add `maximise_data `to the other classes later.

4. info doesn't record the setting.` _create_info` doesn't store `maximise_data`, so you can't tell from info which mode was used. `concurrent_period `just changes meaning.

Suggested next steps

1. Make the validation check respect `maximise_data`: require at least two valid heights at some timestamp when it's True. I suggest to address as part of this PR.

2. store on create_info` the`maximise_data` input.  I suggest to address as part of this PR.

3. Pass the mode through to the plot, or document that the plot always uses only timestamps with every height valid. Should this be addressed as part of [bw.Shear.Timeseries - Timeseries calculates Shear twice and generates plot based on Average method #341](https://github.com/brightwind-dev/brightwind/issues/341) ?

4. Remove the unused maximise_data branches in _calc_power_law/_calc_log_law or add a separate enhancement issue to use maximise_data functionality of _calc_power_law/_calc_log_law also on `Average`, `BySector `and `TimeOfDay`.  One or the other option should be just tracked on a issue for future work as this is very low priority to address. It is just good to have it on the brightwind issue list for future work. I leave @stephenholleran decide if he prefers to remove the unused `maximise_data ` or enhance `Average`, `BySector `and `TimeOfDay` to use this option.

@stephenholleran when point 1 and 2 on 'suggested next steps' are addressed then I am happy for you to approve.

Thanks @BiancaMorandi. Addressed suggested next steps 1 and 2:

1. Validation respects maximise_data: with maximise_data=True, _valid_wsp_data_error_msg now only requires at least one timestamp with two or more heights valid (not NaN and above min_speed). A single height that is entirely NaN no longer raises in this mode. Behaviour with maximise_data=False is unchanged.
2. info records the setting: TimeSeries.info['input data']['maximise_data'] now stores the input. Other classes' info is unchanged.

Tests added in test_shear.py for both.

One caveat: TimeSeries(..., maximise_data=True) will still raise "no valid data" on a dataset where the heights never all overlap. This is because the profile plot is built with Shear.Average, which only uses timestamps where all heights are valid (your gap 2). The validation itself is fixed, but the full call only works on such data once the plot follows maximise_data. I propose handling this in #341

[BiancaMorandi]

Thanks @olivia-bentley . I have added comment about point 3 on #341.

I leave you discuss whit @stephenholleran what is the option he prefers for my point 4 below and add the relevant issue.

4. Remove the unused maximise_data branches in _calc_power_law/_calc_log_law or add a separate enhancement issue to use maximise_data functionality of _calc_power_law/_calc_log_law also on Average, BySector and TimeOfDay. One or the other option should be just tracked on a issue for future work as this is very low priority to address. It is just good to have it on the brightwind issue list for future work. I leave @stephenholleran decide if he prefers to remove the unused maximise_data  or enhance Average, BySector and TimeOfDay to use this option.

@stephenholleran this PR is ready for your approval.