Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add upsampling to the client #145

Open
wants to merge 4 commits into
base: v0.x.x
Choose a base branch
from

Conversation

TalweSingh
Copy link
Contributor

This PR introduces upsampling to the client and adds test to check this functionality.

@TalweSingh TalweSingh requested a review from a team as a code owner February 8, 2025 22:49
@github-actions github-actions bot added the part:docs Affects the documentation label Feb 8, 2025
@TalweSingh TalweSingh force-pushed the resample branch 2 times, most recently from b78d433 to f761662 Compare February 8, 2025 22:59
@shsms shsms requested a review from cwasicki February 10, 2025 08:47
Copy link
Contributor

@shsms shsms left a comment

Choose a reason for hiding this comment

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

Didn't look into the resampling code.

Comment on lines 350 to 354
raise ValueError(
f"Original period {original_period} must be positive non-zero"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
raise ValueError(
f"Original period {original_period} must be positive non-zero"
)
raise ValueError(
f"Original period {original_period} must be greater than zero"
)

f"number of features ({len(features)})"
)

# Validate periods are positive non-zero
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Validate periods are positive non-zero
# Validate periods are greater than zero

Comment on lines 354 to 361
# Validate target period divides evenly into original period
if original_period.total_seconds() % target_period.total_seconds() != 0:
raise ValueError(
f"Target period {target_period} must divide evenly into "
f"original period {original_period}"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe like this?

Suggested change
# Validate target period divides evenly into original period
if original_period.total_seconds() % target_period.total_seconds() != 0:
raise ValueError(
f"Target period {target_period} must divide evenly into "
f"original period {original_period}"
)
# Validate target period is an exact divisor of the original period
if original_period.total_seconds() % target_period.total_seconds() != 0:
raise ValueError(
f"Target period {target_period} must be an exact divisor "
f"of the original period {original_period}."
)

array: np.ndarray[
tuple[typing.Any, typing.Any, typing.Any], np.dtype[np.float64]
],
target_period: dt.timedelta,
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of the target period, you can pass in the requested timestamps. That simplifies the code below, allows arbitrarily sampled data and would align with what the user requests from the client. Namely they would request weather forecasts for specific datetime objects which are 15 min resampled. In this approach you first extract the target period (which could fail if it's not equidistant) and then re-construct the timestamps below for resampling.

Copy link
Contributor

Choose a reason for hiding this comment

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

Afterwards I think this whole function could be simplified roughly like the following:

First resampling

def resample(forecasts, validity_times, new_timestamps, axis=0):
    vts = np.array([t.timestamp() for t in validity_times])
    nts = np.array([t.timestamp() for t in new_timestamps])

    resampled = np.apply_along_axis(
        lambda arr: np.interp(nts, vts, arr),
        axis=axis, 
        arr=forecasts
    )

    return resampled

Then step shifting the solar feature indices (idxs):

def shift_idxs(arr, steps, idxs):
    # Modifies in place and bfills
    arr[steps:, :, idxs] = arr[:-steps, :, idxs]
    arr[:steps, :, idxs] = arr[steps:steps+1, :, idxs]
    return arr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
part:docs Affects the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants