search

NREL / GEOPHIRES-X

MIT License
28 stars 24 forks source link

CoolProp integration; Pressure-aware water property calculations (v3.4.2) #121

Closed softwareengineerprogrammer closed 7 months ago

softwareengineerprogrammer commented 7 months ago

Self-reviewed sub-PRs:

  1. https://github.com/softwareengineerprogrammer/GEOPHIRES-X/pull/10
    1. Addresses https://github.com/NREL/GEOPHIRES-X/issues/113
  2. https://github.com/softwareengineerprogrammer/GEOPHIRES-X/pull/11
    1. https://github.com/NREL/GEOPHIRES-X/issues/110
    2. Fixes bugs related to resevoir hydrostatic pressure, discovered in the course of implementation
    3. Also relevant to https://github.com/NREL/GEOPHIRES-X/issues/118
  3. Addresses https://github.com/NREL/GEOPHIRES-X/issues/107
  4. Identifies and partially addresses https://github.com/NREL/GEOPHIRES-X/issues/120

Reviewers are encouraged to look at diff to example outputs (https://github.com/NREL/GEOPHIRES-X/pull/121/files#diff-34b4448aead45f08341092fe535fc9270bcf1d8420c517)

Deployed and verified on backend/UI (https://jonathanpezzino.com/geothermal/geophires)

Screenshot 2024-02-14 at 16 32 15

softwareengineerprogrammer commented 7 months ago

Looks good, but I don't understand the new construct on Model.py: self.reserv: Reservoir = TDPReservoir(self)

The addition of : Reservoir is a type hint. In this case I added the hint because it enabled PyCharm to give me better autocomplete suggestions as I was working on code that referenced model.reserv (i.e. typing model.reserv. and then being presented with suggestions for .value, .quantity(), etc.).

In general, we should ideally be using/adding type hints for all code as best practice since they improve contributor experience, code comprehensibility, and serve as a guardrail against many common classes of programming errors. I have created a tracking item for this: https://github.com/NREL/GEOPHIRES-X/issues/100. However, I am deferring on adding types everywhere for a few reasons for now:

  1. Adding type hints everywhere all at once would be a major task with limited immediate functional benefit and lots of risk for breakage/issues. The short-term ROI isn't worth it given current resourcing and priorities (although I look forward to the day/time that it will be)
  2. The work would entail adding a type linter so that type hinting is automatically enforced instead of relying on contributors to remember on their own - similar to how pre-commit is enabled for styling. Setting up type linting (in addition to type hinting) adds significant work

So for now, I am just doing the following:

  1. Adding type hints to new code that I add (at least when I remember - hence the value of the linter)
  2. Adding type hints to existing code where there is immediate benefit relevant to the work I'm doing (such as enabling autocomplete that helped me implement this PR more efficiently)
malcolm-dsider commented 7 months ago

It seems logical, and I will endeavor to follow your suggestions below about when to add type hints… but I clearly don’t know how to add the proper type hints in all cases…

From: Jonathan Pezzino @.> Sent: Thursday, February 15, 2024 10:39 AM To: NREL/GEOPHIRES-X @.> Cc: Malcolm Ross @.>; Review requested @.> Subject: Re: [NREL/GEOPHIRES-X] CoolProp integration; Pressure-aware water property calculations (v3.4.2) (PR #121)

Looks good, but I don't understand the new construct on Model.py: self.reserv: Reservoir = TDPReservoir(self)

The addition of : Reservoir is a type hint. In this case I added the hint because it enabled PyCharm to give me better autocomplete suggestions as I was working on code that referenced model.reserv (i.e. typing model.reserv. and then being presented with suggestions for .value, .quantity(), etc.).

In general, we should ideally be using/adding type hints for all code as best practice since they improve contributor experience, code comprehensibility, and serve as a guardrail against many common classes of programming errors. I have created a tracking item for this: #100 https://github.com/NREL/GEOPHIRES-X/issues/100 . However, I am deferring on adding types everywhere for a few reasons for now:

  1. Adding type hints everywhere all at once would be a major task with limited immediate functional benefit and lots of risk for breakage/issues. The short-term ROI isn't worth it given current resourcing and priorities (although I look forward to the day/time that it will be)
  2. The work would entail adding a type linter so that type hinting is automatically enforced instead of relying on contributors to remember on their own - similar to how pre-commit is enabled for styling. Setting up type linting (in addition to type hinting) adds significant work

So for now, I am just doing the following:

  1. Adding type hints to new code that I add (at least when I remember - hence the value of the linter)
  2. Adding type hints to existing code where there is immediate benefit relevant to the work I'm doing (such as enabling autocomplete that helped me implement this PR more efficiently)

— Reply to this email directly, view it on GitHub https://github.com/NREL/GEOPHIRES-X/pull/121#issuecomment-1946531796 , or unsubscribe https://github.com/notifications/unsubscribe-auth/AWGVYT5AKQHOJ2DPXSRKAFTYTY2ZVAVCNFSM6AAAAABDJKGS4OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNBWGUZTCNZZGY . You are receiving this because your review was requested. https://github.com/notifications/beacon/AWGVYT3IIPY5CRMGI7KPLUDYTY2ZVA5CNFSM6AAAAABDJKGS4OWGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTTUAW35I.gif Message ID: @. @.> >