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

Forward osmosis unit model and Trevi's system flowsheet #114

Merged
merged 52 commits into from
Nov 21, 2024

Conversation

Zhuoran29
Copy link
Collaborator

This PR integrates the forward osmosis (FO) model developed from the Trevi System's pilot project and includes the following features:

  • A property package describing the proprietary draw solution properties used by Trevi
  • An FO unit model and its associated test file
  • An example flowsheet of building Trevi's FO configuration
  • A cost model based upon the Trevi's system costing numbers
  • Documentation of both the unit model and example flowsheet

@kurbansitterley
Copy link
Contributor

@Zhuoran29 can you check the failing tests? Just updated the repo to use WaterTAP v 1.0 so could be related to that.

Copy link
Contributor

@kurbansitterley kurbansitterley left a comment

Choose a reason for hiding this comment

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

haven't reviewed all the content yet, but left a few comments to fix before then.

@Zhuoran29
Copy link
Collaborator Author

@Zhuoran29 can you check the failing tests? Just updated the repo to use WaterTAP v 1.0 so could be related to that.

@kurbansitterley , Sorry for the late response. Yes, I've been looking into it and it seemed to need some more efforts to solve the convergence issue (seems related to an expr_if-like expression, even when scaling factor and jacobian matrix both look good). I'll try to fix it asap.

@kurbansitterley
Copy link
Contributor

@Zhuoran29 can you check the failing tests? Just updated the repo to use WaterTAP v 1.0 so could be related to that.

@kurbansitterley , Sorry for the late response. Yes, I've been looking into it and it seemed to need some more efforts to solve the convergence issue (seems related to an expr_if-like expression, even when scaling factor and jacobian matrix both look good). I'll try to fix it asap.

If it is related to the expr_if, you should do a totally fresh install of your environment. There was a compatibility issue with a previous version of Pyomo that has since been fixed. So I'd recommend checking if that solves your problem first before going too much further down the rabbit hole.

@Zhuoran29 Zhuoran29 closed this Jul 24, 2024
@Zhuoran29 Zhuoran29 reopened this Jul 24, 2024
Copy link
Contributor

@kurbansitterley kurbansitterley left a comment

Choose a reason for hiding this comment

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

Overall I am very impressed with this huge PR and think it is in great shape. There are a few code things to address and comments I left.

@Zhuoran29
Copy link
Collaborator Author

@kurbansitterley , thanks for the constructive suggestions. I've modified some of them and responded to those that I didn't change. Please feel free to follow with further comments.

@kurbansitterley kurbansitterley added the Priority:High Normal Priority Issue or PR label Sep 25, 2024
Copy link
Contributor

@kurbansitterley kurbansitterley left a comment

Choose a reason for hiding this comment

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

Great job @Zhuoran29

@kurbansitterley kurbansitterley merged commit 841bb60 into watertap-org:main Nov 21, 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 treatment_models
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants