Quantify wind speed shift #42
Conversation
- Moves Operational Curve shift calculations into their own module - Refactor into separate curve specific functions (with underlying more generic private functions) This change improves readability and ease of adding further curve shift calculations.
In similar way that power curve, rpm and pitch shifts are already calculated, this change adds wind speed shift to the ops curve shift module
Display the sign (+/-) of the curve shift value in the log warning message for clarity of the actual shift rather than just displaying the abs() value that is used when determining whether to raise a log warning
samuelwnaylor
force-pushed
the
quantify-wind-speed-shift
branch
from
a56d21c to
4fac54c
Compare
There was a problem hiding this comment.
I am seeing some differences in integration tests, will email you with further detail. I might have found the reasons (see comments) but not totally sure. Apologies there were not unit tests for the original check_for_ops_curve_shift function you are replacing
| if abs(results_dict[descr]) > warn_thresh: | ||
| if warning_msg is None: | ||
| warning_msg = f"{wtg_name} check_for_ops_curve_shift warnings:" | ||
| warning_msg += f" abs({descr}) > {warn_thresh}: {abs(results_dict[descr]):.3f}" | ||
| if warning_msg is not None: | ||
| result_manager.warning(warning_msg) |
There was a problem hiding this comment.
this logic is valuable to keep so that ops curve shifts result in a single consolidated warning, please port it to ops_curve_shift.py
There was a problem hiding this comment.
Addressed by storing warning messages and compiling them
| pre_dropna_df = pre_df.dropna(subset=[scada_ws_col, pw_col, pt_col, rpm_col]).copy() | ||
| post_dropna_df = post_df.dropna(subset=[scada_ws_col, pw_col, pt_col, rpm_col]).copy() |
There was a problem hiding this comment.
I am not completely sure but I think this dropna logic is missing in the new ops_curve_shift.py
There was a problem hiding this comment.
I am not completely sure but I think this dropna logic is missing in the new
ops_curve_shift.py
This logic is here now
wind-up/wind_up/ops_curve_shift.py
Line 71 in 173f074
I've added NaN handling to the tests too...
Required columns are necessary for all curve plot types. The required columns for any curve plot are wind_speed, power, pitch and rpm
samuelwnaylor
force-pushed
the
quantify-wind-speed-shift
branch
from
fea2710 to
e6eefe7
Compare
samuelwnaylor
force-pushed
the
quantify-wind-speed-shift
branch
from
e02387c to
eff9dfd
Compare
samuelwnaylor
force-pushed
the
quantify-wind-speed-shift
branch
from
eff9dfd to
6045c39
Compare
There was a problem hiding this comment.
Maybe I am misunderstanding the commit history but it seems there are no test case changes associated with this fix 6045c39. If that's the case then I think a test relevant to this logic needs adding, ie at least one test should fail before this commit and pass after it
wind_up/ops_curve_shift.py
Outdated
| post_df["expected_y"] = np.interp(post_df[conf.x_col], mean_curve["x_mean"], mean_curve["y_mean"]) | ||
| mean_df = post_df.mean() | ||
|
|
||
| if conf.name in [CurveTypes.PITCH.value, CurveTypes.PITCH]: |
There was a problem hiding this comment.
It seems like it would be better if there were exactly 1 way to identify if the curve shift calculation is for pitch. What about give conf a curvetype attribute which is type CurveTypes so you would have if conf.curve_type == CurveTypes.PITCH:
There was a problem hiding this comment.
Since CurveTypes inherits from str and Enum, then we could simply it to if conf.name == CurveTypes.PITCH.
The tests were
The test case did not change because by chance the test case had I'll add different strings for |
No description provided.