-
Notifications
You must be signed in to change notification settings - Fork 234
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
Initial REE Costing Framework #1238
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1238 +/- ##
==========================================
- Coverage 76.84% 76.78% -0.06%
==========================================
Files 390 391 +1
Lines 61867 62321 +454
Branches 11388 11473 +85
==========================================
+ Hits 47539 47855 +316
- Misses 11866 11959 +93
- Partials 2462 2507 +45
☔ View full report in Codecov by Sentry. |
@@ -64,3 +64,9 @@ def load_sCO2_costing_dictionary(): | |||
with open(os.path.join(directory, "sCO2_costing_parameters.json"), "r") as file: | |||
sCO2_costing_params = json.load(file) | |||
return sCO2_costing_params | |||
|
|||
|
|||
def load_REE_costing_dictionary(): |
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.
Is this the right place for this? It seems really odd that something to do with REEs is ending up in the power plant sub-folder. This might indicate that the power_generation/costing
folder really needs to be moved out to a models_extra/costing
as it has spread beyond just power generation.
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.
@andrewlee94 I am open to a discussion on reorganizing the costing code, this is just an initial implementation and the team decided to leverage our existing framework as a "path of least resistance" in the first pass. The costing folder probably does need to move to support more generic types and we could rename some files (e.g. power_plant_capcost.py
>>> plant_capcost.py
) to reflect our new capabilities.
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 think it really should be moved before this is merged - otherwise it is likely to linger. Seeing as this is our public repo, we should make an effort to ensure that it is user friendly.
__author__ = "Costing Team (B. Paul and M. Zamarripa)" | ||
__version__ = "1.0.0" | ||
|
||
import pyomo.environ as pyo |
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.
Again, is this the right place for this? For one, it is rather isolated at the moment, and it appears to be more of a minimal test case for costing than an actual flowsheet (thus the name would probably be misleading to users). Would this be better placed in the PrOMMiS workspace, which is where the first process flowsheet will go as well (at least until we have something more comprehensive to show to the world)?
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.
If the PrOMMiS workspace is ready, I can push the script there once it is complete and remove it from this PR.
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.
Closing this in favor of https://github.com/prommis/workspace/pull/13 |
Fixes
Summary/Motivation:
Initial framework for REE costing as part of PrOMMiS cost estimation efforts.
Changes proposed in this PR:
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: