-
Notifications
You must be signed in to change notification settings - Fork 25
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
Added additional estimates from GR website #247
Conversation
…date Util_eff to allow for temperatures above 373 by assuming efficiency os the same, given the plant is the same (usually flash)
… the wrong reference.
# Conflicts: # src/geophires_x/CylindricalReservoir.py # src/geophires_x/Economics.py # src/geophires_x/GeoPHIRESUtils.py # src/geophires_x/Reservoir.py # tests/geophires_x_tests/test_reservoir.py
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.
Build is failing - https://github.com/NREL/GEOPHIRES-X/actions/runs/9685364851/job/26725280505?pr=247
Unit tests will also fail once syntax error is addressed since calculations are changed but examples are not updated
DefaultValue=1.0, | ||
Min=0.0, | ||
Max=10.0, | ||
UnitType=Units.PERCENT, | ||
PreferredUnits=PercentUnit.TENTH, | ||
CurrentUnits=PercentUnit.TENTH, | ||
ErrMessage="assume default cylindrical reservoir radius of effect reduction factor (0.1)", | ||
ErrMessage="assume default cyclindrical reservoir radius of effect reduction factor (0.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.
Typo "cyclindrical"
@@ -92,13 +96,14 @@ def __init__(self, model: Model): | |||
) | |||
self.RadiusOfEffectFactor = self.ParameterDict[self.RadiusOfEffectFactor.Name] = floatParameter( | |||
"Cylindrical Reservoir Radius of Effect Factor", | |||
value=1.0, |
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.
Remove - only DefaultValue
should be specified, not value
(which is redundant with DefaultValue
in this case anyway)
@@ -80,6 +83,7 @@ def __init__(self, model: Model): | |||
) | |||
self.RadiusOfEffect = self.ParameterDict[self.RadiusOfEffect.Name] = floatParameter( | |||
"Cylindrical Reservoir Radius of Effect", | |||
value=30.0, |
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.
Remove - only DefaultValue
should be specified, not value
(which is redundant with DefaultValue
in this case anyway)
@@ -68,6 +70,7 @@ def __init__(self, model: Model): | |||
) | |||
self.Length = self.ParameterDict[self.Length.Name] = floatParameter( | |||
"Cylindrical Reservoir Length", | |||
value=4.0, |
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.
Remove - only DefaultValue
should be specified, not value
(which is redundant with DefaultValue
in this case anyway)
@@ -56,6 +57,7 @@ def __init__(self, model: Model): | |||
|
|||
self.OutputDepth = self.ParameterDict[self.OutputDepth.Name] = floatParameter( | |||
"Cylindrical Reservoir Output Depth", | |||
value=self.InputDepth.value, |
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.
Remove - only DefaultValue
should be specified, not value
(which is redundant with DefaultValue
in this case anyway)
f.write(f' Estimated Property tax that will be paid: {model.economics.property_tax_created.value:10.2f }' + model.economics.property_tax_created.PreferredUnits.value + NL) | ||
f.write(f' Estimated total royalties that will be paid: {model.economics.total_royalties_created.value:10.2f }' + model.economics.total_royalties_created.PreferredUnits.value + NL) | ||
f.write(f' Estimated government royalties to be paid: {model.economics.gov_royalties_created.value:10.2f }' + model.economics.gov_royalties_created.PreferredUnits.value+ NL) |
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 there better/more precise phrasing that can eliminate the long and clunky "to be paid"/"that will be paid" suffixes here?
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 will work on the phrasing
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.
But do you like the additional calculations based on the information used to generate the estimate of "jobs created?"
UnitType=Units.JOBS, | ||
CurrentUnits=JobsUnit.JOBS, | ||
PreferredUnits=JobsUnit.JOBS |
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 don't see the value in defining a specific unit for this - counts are unitless and represented as Units.NONE
i.e.
UnitType=Units.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.
Yah, I was struggling with the code during the class, so I added this just to get it run. I will set it back to what is right - but this code does work... just doesn't make alot of sense.
LCOH = self.LCOH.value * 2.931 # $/MMBTU | ||
LCOH = LCOH * 2.931 # $/MMBTU |
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 change doesn't seem possible without impacting example outputs, but no example output changes are in the 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.
This was a one line bug I found some weeks ago and committed but forgot to do a PR, so it got folded into this PR. This bug showed itself in only a couple of cases, but the fix is correct. There may be some output changes, but maybe not.
pressure=self.hydrostatic_pressure() | ||
pressure=model.reserv.lithostatic_pressure() | ||
) | ||
self.rhowater.value = density_water_kg_per_m3( | ||
model.wellbores.Tinj.value * 0.5 + (self.Trock.value * 0.9 + model.wellbores.Tinj.value * 0.1) * 0.5, | ||
pressure=self.hydrostatic_pressure() | ||
pressure=model.reserv.lithostatic_pressure() |
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.
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 had merge conflicts during the class which I had to resolve quickly. We should go back to hydrostatic.
class RoyaltyPerEnergyUnit(str,Enum): | ||
"""Royalty per energy Units""" | ||
ROYALTYPERMW = "MUSD/MW" | ||
ROYALTYPERKW = "MUSD/kW" |
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 is redundant and/or inconsistent with EnergyCostUnit
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 don't think so, but I will check.
Thanks for taking a look. Class is over, so we will deal with the issues when I get time.
From: Jonathan Pezzino ***@***.***>
Sent: Wednesday, June 26, 2024 2:37 PM
To: NREL/GEOPHIRES-X ***@***.***>
Cc: Malcolm Ross ***@***.***>; Author ***@***.***>
Subject: Re: [NREL/GEOPHIRES-X] Added additional estimates from GR website (PR #247)
@softwareengineerprogrammer requested changes on this pull request.
Build is failing - https://github.com/NREL/GEOPHIRES-X/actions/runs/9685364851/job/26725280505?pr=247
Unit tests will also fail once syntax error is addressed since calculations are changed but examples are not updated
_____
In src/geophires_x/CylindricalReservoir.py <#247 (comment)> :
DefaultValue=1.0,
Min=0.0,
Max=10.0,
UnitType=Units.PERCENT,
PreferredUnits=PercentUnit.TENTH,
CurrentUnits=PercentUnit.TENTH,
- ErrMessage="assume default cylindrical reservoir radius of effect reduction factor (0.1)",
+ ErrMessage="assume default cyclindrical reservoir radius of effect reduction factor (0.1)",
Typo "cyclindrical"
_____
In src/geophires_x/CylindricalReservoir.py <#247 (comment)> :
@@ -92,13 +96,14 @@ def __init__(self, model: Model):
)
self.RadiusOfEffectFactor = self.ParameterDict[self.RadiusOfEffectFactor.Name] = floatParameter(
"Cylindrical Reservoir Radius of Effect Factor",
+ value=1.0,
Remove - only DefaultValue should be specified, not value (which is redundant with DefaultValue in this case anyway)
_____
In src/geophires_x/CylindricalReservoir.py <#247 (comment)> :
@@ -80,6 +83,7 @@ def __init__(self, model: Model):
)
self.RadiusOfEffect = self.ParameterDict[self.RadiusOfEffect.Name] = floatParameter(
"Cylindrical Reservoir Radius of Effect",
+ value=30.0,
Remove - only DefaultValue should be specified, not value (which is redundant with DefaultValue in this case anyway)
_____
In src/geophires_x/CylindricalReservoir.py <#247 (comment)> :
@@ -68,6 +70,7 @@ def __init__(self, model: Model):
)
self.Length = self.ParameterDict[self.Length.Name] = floatParameter(
"Cylindrical Reservoir Length",
+ value=4.0,
Remove - only DefaultValue should be specified, not value (which is redundant with DefaultValue in this case anyway)
_____
In src/geophires_x/CylindricalReservoir.py <#247 (comment)> :
@@ -56,6 +57,7 @@ def __init__(self, model: Model):
self.OutputDepth = self.ParameterDict[self.OutputDepth.Name] = floatParameter(
"Cylindrical Reservoir Output Depth",
+ value=self.InputDepth.value,
Remove - only DefaultValue should be specified, not value (which is redundant with DefaultValue in this case anyway)
_____
In src/geophires_x/CylindricalReservoir.py <#247 (comment)> :
@@ -43,6 +43,7 @@ def __init__(self, model: Model):
self.InputDepth = self.ParameterDict[self.InputDepth.Name] = floatParameter(
"Cylindrical Reservoir Input Depth",
+ value=3.0,
Remove - only DefaultValue should be specified, not value (which is redundant with DefaultValue in this case anyway)
_____
In src/geophires_x/CylindricalReservoir.py <#247 (comment)> :
@@ -109,6 +114,7 @@ def __init__(self, model: Model):
# internal values required for calculations
self.depth = self.ParameterDict[self.depth.Name] = floatParameter(
"Drilled length",
+ value=0.0,
Remove - only DefaultValue should be specified, not value (which is redundant with DefaultValue in this case anyway)
_____
In src/geophires_x/Outputs.py <#247 (comment)> :
@@ -1641,7 +1641,11 @@ def PrintOutputs(self, model: Model):
f.write(f' CHP: Percent cost allocation for electrical plant: {model.economics.CAPEX_heat_electricity_plant_ratio.value*100.0:10.2f} %\n')
if model.surfaceplant.enduse_option.value in [EndUseOptions.ELECTRICITY]:
- f.write(f' Estimated Jobs Created: {model.economics.jobs_created.value}\n')
+ f.write(f' Estimates from https://geothermal.org/resources/geothermal-basics:' + NL)
Revert to Estimated Jobs Created
1. This would be a backwards-incompatible field name change
2. Although I like the idea of citing on a per-field basis, we shouldn't include citations in field names as a general principle since this would make output very difficult to read. Rather, we should include citations in documentation and/or a documentation metadata field - filed tracking issue #248 <#248>
3. This specific citation is already present in the parameter description https://nrel.github.io/GEOPHIRES-X/parameters.html#id11 -> ctrl+F "Estimated Jobs Created per MW of Electricity Produced"
_____
In src/geophires_x/Outputs.py <#247 (comment)> :
+ f.write(f' Estimated Property tax that will be paid: {model.economics.property_tax_created.value:10.2f }' + model.economics.property_tax_created.PreferredUnits.value + NL)
+ f.write(f' Estimated total royalties that will be paid: {model.economics.total_royalties_created.value:10.2f }' + model.economics.total_royalties_created.PreferredUnits.value + NL)
+ f.write(f' Estimated government royalties to be paid: {model.economics.gov_royalties_created.value:10.2f }' + model.economics.gov_royalties_created.PreferredUnits.value+ NL)
Is there better/more precise phrasing that can eliminate the long and clunky "to be paid"/"that will be paid" suffixes here?
_____
In src/geophires_x/Economics.py <#247 (comment)> :
+ UnitType=Units.JOBS,
+ CurrentUnits=JobsUnit.JOBS,
+ PreferredUnits=JobsUnit.JOBS
I don't see the value in defining a specific unit for this - counts are unitless and represented as Units.NONE i.e. https://github.com/NREL/GEOPHIRES-X/blob/28df988d6f004c400239383e1f48e2b4bed9adb8/src/geophires_x/SUTRAWellBores.py#L57
_____
In src/geophires_x/Economics.py <#247 (comment)> :
- LCOH = self.LCOH.value * 2.931 # $/MMBTU
+ LCOH = LCOH * 2.931 # $/MMBTU
This change doesn't seem possible without impacting example outputs, but no example output changes are in the PR?
_____
In src/geophires_x/CylindricalReservoir.py <#247 (comment)> :
- pressure=self.hydrostatic_pressure()
+ pressure=model.reserv.lithostatic_pressure()
)
self.rhowater.value = density_water_kg_per_m3(
model.wellbores.Tinj.value * 0.5 + (self.Trock.value * 0.9 + model.wellbores.Tinj.value * 0.1) * 0.5,
- pressure=self.hydrostatic_pressure()
+ pressure=model.reserv.lithostatic_pressure()
We just changed these from lithostatic to hydrostatic last month (#217 <#217> / <c77f418> c77f418), is this change back intentional? If so, what's the rationale?
_____
In src/geophires_x/Units.py <#247 (comment)> :
+class RoyaltyPerEnergyUnit(str,Enum):
+ """Royalty per energy Units"""
+ ROYALTYPERMW = "MUSD/MW"
+ ROYALTYPERKW = "MUSD/kW"
This is redundant and/or inconsistent with EnergyCostUnit
—
Reply to this email directly, view it on GitHub <#247 (review)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AWGVYT2CUHEQG3DLWRBVC7TZJMJ5LAVCNFSM6AAAAABJ6PRWT2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCNBSG42DOMZUGM> .
You are receiving this because you authored the thread. <https://github.com/notifications/beacon/AWGVYT4K6MWBR74Z6JXN6GLZJMJ5LA5CNFSM6AAAAABJ6PRWT2WGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTT7W65M6.gif> Message ID: ***@***.*** ***@***.***> >
|
Relevant tracking issue: #169 |
Closing per sync discussion. |
Added estimate for property tax, royalties, and govt royalties
Also other miscellaneous updates
Relevant to #232