-
-
Notifications
You must be signed in to change notification settings - Fork 8
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 datetime features #89
base: main
Are you sure you want to change the base?
Conversation
Ive put this back as a draft, im we perhaps can do this on the file when loading the data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of updates I feel might be worthwhile
return date_in_pi, time_in_pi | ||
|
||
|
||
def make_datetime_numpy_batch(datetimes: pd.DatetimeIndex, key_prefix: str = "wind") -> dict: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potentially update main function to include input validation - also to simplify the dict construction via literal. Feel this improves maintenance ease a little overall. As example:
def make_datetime_numpy_batch(datetimes: pd.DatetimeIndex, key_prefix: str = "wind") -> dict:
"""
Make a dict of datetime features using sin/cos transformations
"""
if datetimes.empty:
raise ValueError("Input datetimes is empty")
# Obtain scaled radians for date and time
date_in_pi, time_in_pi = _get_date_time_in_pi(datetimes)
# Create dict
return {
f"{key_prefix}_date_sin": np.sin(date_in_pi),
f"{key_prefix}_date_cos": np.cos(date_in_pi),
f"{key_prefix}_time_sin": np.sin(time_in_pi),
f"{key_prefix}_time_cos": np.cos(time_in_pi),
}
assert all(np.abs(datetime_features["wind_date_sin"]) <= 1) | ||
assert all(np.abs(datetime_features["wind_date_cos"]) <= 1) | ||
assert all(np.abs(datetime_features["wind_time_sin"]) <= 1) | ||
assert all(np.abs(datetime_features["wind_time_cos"]) <= 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing wise - perhaps update to additionaly verify if a custom prefix is sufficiently applied to all dict keys that the function generates and a further basic check if an empty input corresponds to error raising as implemented in main. As example:
def test_make_datetime_numpy_batch_custom_key_prefix():
# Test function correctly applies custom prefix to dict keys
datetimes = pd.to_datetime(["2024-06-20 12:00", "2024-06-20 12:30", "2024-06-20 13:00"])
key_prefix = "solar"
datetime_features = make_datetime_numpy_batch(datetimes, key_prefix=key_prefix)
# Assert dict contains expected quantity of keys and verify starting with custom prefix
assert len(datetime_features) == 4
assert all(key.startswith(key_prefix) for key in datetime_features.keys())
def test_make_datetime_numpy_batch_empty_input():
# Verification that function raises error for empty input
datetimes = pd.DatetimeIndex([])
with pytest.raises(ValueError, match="Input datetimes is empty"):
make_datetime_numpy_batch(datetimes)
Pull Request
Description
Add datetime features to site
#77
How Has This Been Tested?
CI tests + new tests
Checklist: