malcolm-dsider
commented
6 days ago 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 https://github.com/NREL/GEOPHIRES-X/pull/247#discussion_r1655397885 :
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 https://github.com/NREL/GEOPHIRES-X/pull/247#discussion_r1655398679 :
@@ -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 https://github.com/NREL/GEOPHIRES-X/pull/247#discussion_r1655398712 :
@@ -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 https://github.com/NREL/GEOPHIRES-X/pull/247#discussion_r1655398756 :
@@ -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 https://github.com/NREL/GEOPHIRES-X/pull/247#discussion_r1655398817 :
@@ -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 https://github.com/NREL/GEOPHIRES-X/pull/247#discussion_r1655398856 :
@@ -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 https://github.com/NREL/GEOPHIRES-X/pull/247#discussion_r1655399036 :
@@ -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 https://github.com/NREL/GEOPHIRES-X/pull/247#discussion_r1655407471 :
@@ -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
- This would be a backwards-incompatible field name change
- 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 https://github.com/NREL/GEOPHIRES-X/issues/248
-
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 https://github.com/NREL/GEOPHIRES-X/pull/247#discussion_r1655411312 :
- 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 https://github.com/NREL/GEOPHIRES-X/pull/247#discussion_r1655414100 :
- 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 https://github.com/NREL/GEOPHIRES-X/pull/247#discussion_r1655414941 :
- 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 https://github.com/NREL/GEOPHIRES-X/pull/247#discussion_r1655417288 :
- 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 https://github.com/NREL/GEOPHIRES-X/pull/217 / https://github.com/NREL/GEOPHIRES-X/commit/c77f4183b445f1e1767dd3d62fda68436c7f7acd c77f418), is this change back intentional? If so, what's the rationale?
In src/geophires_x/Units.py https://github.com/NREL/GEOPHIRES-X/pull/247#discussion_r1655420420 :
+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 https://github.com/NREL/GEOPHIRES-X/pull/247#pullrequestreview-2142747343 , 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: @. @.> >
softwareengineerprogrammer
Added estimate for property tax, royalties, and govt royalties
Also other miscellaneous updates
Relevant to https://github.com/NREL/GEOPHIRES-X/issues/232