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

395 implement new deterministic metrics kge #396

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

samland1116
Copy link
Contributor

This PR updates the metrics framework for KGE, KGEmod1, and KGEmod2 to allow for user defined constants. This PR also explores the impact of wrapper functions on query performance. By creating a wrapper around the KGE functions that ingests the KGE base models, we can specify user defined constants like below:

from teehr import DeterministicMetrics

kge = DeterministicMetrics.KlingGuptaEfficiency()
kge.sr = 0.5
kge.sa = 0.5
kge.sb = 0.5

include_metrics = [kge]

metrics_df = ev.metrics.query(
    group_by=["primary_location_id", "configuration_name"],
    include_metrics=include_metrics
).to_pandas()

Alternatively, the existing method to query metrics still works despite the presence of a wrapper, shown below:

from teehr import DeterministicMetrics

metrics_df = ev.metrics.query(
    group_by=["primary_location_id", "configuration_name"],
    include_metrics=[DeterministicMetrics.KlingGuptaEfficiency()]
).to_pandas()

Unexpectedly, the presence of a wrapper that does not alter values within a given function results in somewhat significant performance improvements while the presence of a wrapper that does alter values within a given function results in performance similar to that of our existing metrics implementation.

I propose we implement this functionality on all existing metrics functions to standardize metrics behavior while simultaneously improving metric query performance.

@samland1116 samland1116 added the enhancement New feature or request label Mar 5, 2025
@samland1116 samland1116 added this to the v0.5 Release milestone Mar 5, 2025
@samland1116 samland1116 requested review from samlamont and mgdenno March 5, 2025 18:12
@samland1116 samland1116 self-assigned this Mar 5, 2025
@samland1116 samland1116 linked an issue Mar 5, 2025 that may be closed by this pull request
Copy link
Collaborator

@samlamont samlamont left a comment

Choose a reason for hiding this comment

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

I think it looks go to me overall. I wonder if we should just implement the wrappers for all metrics in this PR since it would get rid of the requires_wrapper checks and allow us to call the pandas_udf in the same way for all metrics?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement new deterministic metrics: KGE
2 participants