Skip to content

Conversation

softwareengineerprogrammer
Copy link
Owner

@softwareengineerprogrammer softwareengineerprogrammer commented Mar 7, 2025

It was brought to my attention by some user feedback that Electricity Provided showed an unexplained dropoff in the final project year:

image

image

Upon investigation, I discovered that this was an artifact of the dx argument to np.trapz used for integrating the Electricity Provided profile from individual time step values incorrectly assuming that the final profile slice had number of members = time steps per year, when the final slice actually has 1 fewer member (in the cases I looked at).

To confirm that this is indeed a bug, conceptually speaking: Consider current behavior when time steps per year is set to 1. In this case, Heat Extracted and Electricity Provided will be zero for the final year. This is obviously wrong, and demonstrates that the current usage of np.trapz is flawed and that the behavior is not intentional:
image

To resolve, I updated usage of np.trapz in SurfaceApplication*.py time series integrations to calculate dx according to the actual number of time steps in the slice, rather than assuming the value is equal to time steps per year.

Testing/verification of fix:

  1. I mitigated against risks associated with refactoring complexity by first refactoring with the original, incorrect behavior, and ensuring the build passed with the unit tests unchanged: https://github.com/softwareengineerprogrammer/GEOPHIRES/actions/runs/13687157605. That is - I checked that the changes in results are due solely to the change in implementation behavior and not errors made in adding/refactoring to use SurfacePlant.integrate_time_series_slice
  2. Regenerated example .out files (per standard procedure) and manually sanity-checked
  3. Added unit tests for {time steps per year} = 1 case:
    1. https://github.com/softwareengineerprogrammer/GEOPHIRES/pull/52/files#diff-5bc5da06375198c1bdfc7002928167bc13527c20313810f78902aa9733f42882R11-R76
    2. https://github.com/softwareengineerprogrammer/GEOPHIRES/pull/52/files#diff-01c5adc6b171191b928e59a6206cad4a36e9f2cec9c939e60b4819d518e36664R680-R712
  4. Manually verified graph for time steps per year = 1 (from above) is corrected:
    image

@softwareengineerprogrammer softwareengineerprogrammer changed the title Surface Plant time series integrations fix (Electricity Provided + other profiles final year value correction) Surface Plant time series integrations fix (Electricity Provided + other profiles final year value correction) [v3.7.22] Mar 10, 2025
@softwareengineerprogrammer softwareengineerprogrammer changed the title Surface Plant time series integrations fix (Electricity Provided + other profiles final year value correction) [v3.7.22] Surface Plant time series integrations fix (Electricity Provided + other profiles final year value correction) Mar 10, 2025
Comment on lines +12 to +39
@staticmethod
def integrate_time_series_slice(
series: np.ndarray,
_i: int,
time_steps_per_year: int,
utilization_factor: float
) -> np.float64:
slice_start_index = _i * time_steps_per_year
slice_end_index = ((_i + 1) * time_steps_per_year) + 1
_slice = list(series[slice_start_index:slice_end_index])

# Note that len(_slice) - 1 may be less than time_steps_per_year for the last slice.

if len(_slice) == 1:
extrapolated_future_datapoint = _slice[0]
if slice_start_index - 1 > 0:
delta = series[slice_start_index] - series[slice_start_index - 1]
extrapolated_future_datapoint = _slice[0] + delta
_slice.append(extrapolated_future_datapoint)

dx_steps = len(_slice) - 1

integral = np.trapz(
_slice,
dx=1. / dx_steps * 365. * 24.
)

return integral * 1000. * utilization_factor
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The crux of this fix is that the previous behavior was doing the equivalent of erroneously setting dx_steps to time_steps_per_year. The last slices of the time series in question always have one less data point than time_steps_per_year, which was causing the trapezoidal rule to behave as if the slice had a 'phantom' entry with value=0 appended to it, thus lowering the estimated integral value of the slice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant