Skip to content
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

KBHDP RPT 2 and Modify REFLOSystemCosting #141

Merged
merged 47 commits into from
Dec 10, 2024

Conversation

zacharybinger
Copy link
Collaborator

@zacharybinger zacharybinger commented Nov 18, 2024

Creating PR for RPT2Case Study

This case study includes EC, UF, LTMED, DWI powered by FPC.

This PR also removes aggregate_flow_heat_sold and aggregate_flow_electricity_sold and associated constraints and tests. These _sold variables variables were always zero in all use cases to date, and this was causing stability and solve issues for these case studies. If it is desired to add these variables/constraints back in to REFLOSystemCosting in the future, a "frozen" version of the previous costing package is available here as a starting point:

https://github.com/kurbansitterley/watertap-reflo/tree/costing_package_freeze

@zacharybinger zacharybinger marked this pull request as ready for review November 22, 2024 16:00
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 10 out of 25 changed files in this pull request and generated 3 suggestions.

Files not reviewed (15)
  • src/watertap_contrib/reflo/analysis/case_studies/KBHDP/components/ro_system.py: Evaluated as low risk
  • src/watertap_contrib/reflo/analysis/case_studies/KBHDP/utils/init.py: Evaluated as low risk
  • src/watertap_contrib/reflo/analysis/case_studies/KBHDP/tests/test_RPT_2.py: Evaluated as low risk
  • src/watertap_contrib/reflo/analysis/case_studies/KBHDP/components/translator_4.py: Evaluated as low risk
  • src/watertap_contrib/reflo/analysis/case_studies/KBHDP/components/translator_3.py: Evaluated as low risk
  • src/watertap_contrib/reflo/analysis/case_studies/KBHDP/components/translator_2.py: Evaluated as low risk
  • src/watertap_contrib/reflo/analysis/case_studies/KBHDP/components/init.py: Evaluated as low risk
  • src/watertap_contrib/reflo/analysis/case_studies/KBHDP/components/deep_well_injection.py: Evaluated as low risk
  • src/watertap_contrib/reflo/core/solar_energy_base.py: Evaluated as low risk
  • src/watertap_contrib/reflo/analysis/case_studies/KBHDP/components/PV.py: Evaluated as low risk
  • src/watertap_contrib/reflo/costing/watertap_reflo_costing_package.py: Evaluated as low risk
  • src/watertap_contrib/reflo/analysis/case_studies/KBHDP/init.py: Evaluated as low risk
  • src/watertap_contrib/reflo/data/technoeconomic/ultra_filtration.yaml: Evaluated as low risk
  • src/watertap_contrib/reflo/analysis/case_studies/KBHDP/components/UF.py: Evaluated as low risk
  • src/watertap_contrib/reflo/data/technoeconomic/electrocoagulation.yaml: Evaluated as low risk
Comments skipped due to low confidence (2)

src/watertap_contrib/reflo/analysis/case_studies/KBHDP/components/FPC.py:143

  • The blk parameter is used but not defined in the function signature. It should be added to the function signature.
def add_FPC_scaling(m, blk):

src/watertap_contrib/reflo/analysis/case_studies/KBHDP/components/LTMED.py:62

  • [nitpick] The function name _initialize starts with an underscore, which is typically used for private methods. Since this function is used in other parts of the code, it should be renamed to initialize to avoid confusion.
def _initialize(blk, verbose=False):

else:
print("\n--------- FAILED SOLVE!!! ---------\n")
print(msg)
assert False
Copy link
Preview

Copilot AI Nov 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assertion assert False will always fail. It should be removed or replaced with appropriate error handling.

Suggested change
assert False
return None

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options


def calc_scale(value):
if value != 0 or value != None:
Copy link
Preview

Copilot AI Nov 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The condition should be 'if value != 0 and value is not None:' to correctly handle None values.

Suggested change
if value != 0 or value != None:
if value != 0 and value is not None:

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
@adam-a-a adam-a-a added the Priority:High Normal Priority Issue or PR label Dec 9, 2024
@kurbansitterley kurbansitterley changed the title RPT 2 KBHDP RPT 2 Dec 9, 2024
@kurbansitterley kurbansitterley changed the title KBHDP RPT 2 KBHDP RPT 2 and Modify REFLOSystemCosting Dec 10, 2024
@kurbansitterley kurbansitterley merged commit bc2abcf into watertap-org:main Dec 10, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:High Normal Priority Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants