-
Notifications
You must be signed in to change notification settings - Fork 261
Add Clustering Method to PriceTaker #1579
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1579 +/- ##
==========================================
- Coverage 76.86% 76.79% -0.07%
==========================================
Files 394 394
Lines 63246 63294 +48
Branches 10360 10369 +9
==========================================
- Hits 48611 48609 -2
- Misses 12187 12238 +51
+ Partials 2448 2447 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
idaes/apps/grid_integration/pricetaker/tests/test_clustering.py
Outdated
Show resolved
Hide resolved
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.
I have a few changes. Please see my comments below.
@@ -31,7 +30,7 @@ | |||
@pytest.fixture(name="sample_data") | |||
def sample_data_fixture(): | |||
"""Returns a sample price signal for testing""" | |||
file_path = Path(__file__).parent / "FLECCS_princeton.csv" | |||
file_path = "../../multiperiod/tests/lmp_data.csv" |
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.
The lmp_data.csv
file is not available in the specified path. Please include the test file in the pricetaker/tests
folder. Also, please ensure that the file contains only one price signal that is needed for the test.
up_time=up_time, | ||
down_time=down_time, | ||
minimum_up_time=minimum_up_time, | ||
minimum_down_time=minimum_down_time, |
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.
Please update the argument names in test_price_taker_model.py
, unit_commitment.py
and test_unit_commitment.py
files.
@@ -803,7 +803,7 @@ def add_overall_cashflows( | |||
lifetime: int = 30, | |||
discount_rate: float = 0.08, | |||
corporate_tax_rate: float = 0.2, | |||
annualization_factor: Optional[float] = None, | |||
capital_recovery_factor: Optional[float] = None, |
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.
For now, please retain annualization_factor
as the argument name. I do not think this value always indicates capital_recovery_factor
, so we need to come up with a better name for this argument.
|
||
if len(local_maxima) == 0: | ||
n_clusters = 0 | ||
_logger.warning( |
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.
Instead of warning, raise an error: Optimal number of clusters cannot be determined with the elbow method for this dataset. Try silhouette method instead.
Summary/Motivation:
Adds a literature-based method to the pricetaker model for determining the optimal number of clusters that a dataset should be broken into
Changes proposed in this PR:
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: