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

Minor Dye Desalination Flowsheet Updates #1349

Merged
merged 8 commits into from
Apr 3, 2024

Conversation

MarcusHolly
Copy link
Contributor

@MarcusHolly MarcusHolly commented Mar 29, 2024

Fixes/Resolves:

Resolves various comments pointed out in #1294; namely:

  • Replacing instances of .value with value()
  • Adding a product block to reduce the number of if-statements
  • Add config option for toggling between 1DRO and 0DRO

Legal Acknowledgement

By contributing to this software project, I agree to the following terms and conditions for my contribution:

  1. I agree my contributions are submitted under the license terms described in the LICENSE.txt file at the top level of this directory.
  2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

@MarcusHolly MarcusHolly self-assigned this Mar 29, 2024
@MarcusHolly
Copy link
Contributor Author

I have not combined the dye_desalination_withRO.py and dye_desalination.py flowsheets in this PR (i.e. merging into one flowsheet that has a configuration option to include the RO treatment train). I've started implementing this change locally, but it's a bigger hassle than I initially thought - mainly because modifying the costing is a pain. So perhaps this smaller PR can be merged first and a subsequent PR will focus on combining the two flowsheets (it should also be easier to review this way).

@MarcusHolly MarcusHolly marked this pull request as ready for review March 29, 2024 20:56
Copy link

codecov bot commented Mar 29, 2024

Codecov Report

Attention: Patch coverage is 90.90909% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 93.84%. Comparing base (11705cb) to head (4b55be3).

Files Patch % Lines
...covery/dye_desalination/dye_desalination_withRO.py 90.90% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1349      +/-   ##
==========================================
- Coverage   93.85%   93.84%   -0.01%     
==========================================
  Files         337      337              
  Lines       35289    35290       +1     
==========================================
  Hits        33119    33119              
- Misses       2170     2171       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@hunterbarber hunterbarber left a comment

Choose a reason for hiding this comment

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

Changes are all very minor so LGTM. I would maybe still bullet the naming conventions, conversion to pyo.value() and other changes in the PR describtion for clarity.

@adam-a-a
Copy link
Contributor

adam-a-a commented Apr 3, 2024

LGTM - I think we can rethink little bits here and there, but we can aim to polish on subsequent PR that combines both flowsheets (with and without RO).

@adam-a-a adam-a-a merged commit c93438e into watertap-org:main Apr 3, 2024
24 checks passed
@ksbeattie ksbeattie added the Priority:Normal Normal Priority Issue or PR label Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:Normal Normal Priority Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants