-
Notifications
You must be signed in to change notification settings - Fork 276
Location factor #1642
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?
Location factor #1642
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1642 +/- ##
==========================================
- Coverage 77.01% 77.01% -0.01%
==========================================
Files 395 395
Lines 63555 63567 +12
Branches 10365 10365
==========================================
+ Hits 48944 48953 +9
- Misses 12171 12175 +4
+ Partials 2440 2439 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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 some questions about the implementation here.
with pytest.raises(StopIteration): | ||
_ = next( | ||
entry | ||
for entry in location_data | ||
if (entry["country"], entry["city"]) == test_location | ||
) |
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 test name indicates that this should raise a KeyError
but the test itself indicates it raises a StopIteration
error. It seems like you should catch the StopIteration
error and raise a KeyError
yourself.
Also, why are we iterating through a list entry-by-entry to check whether the country-city pair exists? Doesn't it make more sense to implement it an object (like a dict
) that uses a hash lookup?
else: | ||
pytest.skip("Test assumptions not satisfied for invalid city test.") |
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.
Why are we skipping the test here? The only reason we should be skipping tests is if the test requires some dependency (like a solver) that not every environment has.
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.
Thanks @Lingyan90 for opening this, I have a few comments.
|
||
def load_location_factor(): | ||
""" | ||
Estimate the cost of constructing the same plant in different global locations using location factors. |
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.
Estimate the cost of constructing the same plant in different global locations using location factors. | |
Estimate the cost of constructing the same plant in different regions using location factors. |
) | ||
|
||
|
||
def load_location_factor(): |
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.
def load_location_factor(): | |
def load_location_factors(): |
self.base_currency = None | ||
self.base_period = pyo.units.year | ||
|
||
# Set the location factor |
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.
# Set the location factor | |
# Set the location factor, default U.S. Washington DC, location_factor = 1.00 |
register_idaes_currency_units, | ||
) | ||
|
||
from idaes.core.base.costing_base import load_location_factor |
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.
from idaes.core.base.costing_base import load_location_factor | |
from idaes.core.base.costing_base import load_location_factors |
|
||
@pytest.mark.unit | ||
def test_all_valid_locations_have_factors(): | ||
location_data = load_location_factor() |
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.
And everywhere else in the file where this appears.
location_data = load_location_factor() | |
location_data = load_location_factors() |
), f"Test assumption failed: {invalid_country} unexpectedly exists in data" | ||
|
||
|
||
@pytest.mark.unit |
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.
How would this work in practice? If a user tried to call a country which an invalid region/city, would it return a default?
register_idaes_currency_units, | ||
) | ||
|
||
from idaes.core.base.costing_base import load_location_factor |
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.
from idaes.core.base.costing_base import load_location_factor | |
from idaes.core.base.costing_base import load_location_factors |
class SSLWCostingData(FlowsheetCostingBlockData): | ||
# Register currency and conversion rates based on CE Index | ||
register_idaes_currency_units() | ||
load_location_factor() |
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.
This should be assigned to an object, so it can be referenced later.
I think "location" should also be a configuration argument like "CE_index_year", so the UnitModelCostingBlock
will save the attribute self.location = ("United States", "Washington DC")
and then apply it to the capital cost equation.
Finally, please add tests to the SSLW test file applying location factors and check built objects, object values, and cost results.
# Set a base period for all operating costs | ||
self.base_period = pyo.units.year | ||
# Chose location and set location factor | ||
self.location_factor = ("United States", "Washington DC") |
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.
self.location_factor = ("United States", "Washington DC") | |
self.location = ("United States", "Washington DC") |
And then internal to the costing method, we can define self.location_factor = location_data[self.location]
.
@Lingyan90, any news on this? |
This PR should be addressed once the PrOMMiS costing dictionary has been moved to IDAES. |
Is there a PR, issue and/or person for that move (in either PrOMMiS or IDAES)? |
@ksbeattie I and others on the costing team will coordinate this move soon. Here is the issue: prommis/prommis#150 |
Fixes
The location factor is an important metric for analyzing different market location. Here, move the location factor from PrOMMiS to IDAES for broader application. (This originally started in PrOMMiS in PR prommis/prommis#143)
Summary/Motivation:
Move the location factor to IDAES so that other repository such as PrOMMiS, WaterTAP, etc. could also access these values.
Changes proposed in this PR:
costing_base.py
location_factors.jason
test_costing_base.py
SSLW.py
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: