-
Notifications
You must be signed in to change notification settings - Fork 62
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
ADM1/ASM2d Translator Update #1435
Merged
adam-a-a
merged 87 commits into
watertap-org:main
from
MarcusHolly:new_adm1_asm2d_interface
Jul 18, 2024
Merged
Changes from all commits
Commits
Show all changes
87 commits
Select commit
Hold shift + click to select a range
af1dc9a
Finish implementation of adm1/asm2d translator
MarcusHolly 8318db2
Resolve inconsistent unit errors
MarcusHolly c0b8329
Update test file
MarcusHolly b1273c4
Remove completed to-do item
MarcusHolly df81af7
Merge branch 'main' into new_adm1_asm2d_interface
MarcusHolly 6b6a448
Add Soraya's scaling improvements
MarcusHolly 66a7c44
Address minor formatting issues
MarcusHolly 07ff096
Add BSM2 flowsheet w/new translator
MarcusHolly 8133b7a
New flowsheet solving with Bio_P=True
MarcusHolly 906fb13
Minor improvements to BSM2
MarcusHolly 4953299
BioP=True solves & BioP=False fails
MarcusHolly f5ee78f
Update translator documentation
MarcusHolly 49ce16c
Correct imports in test file
MarcusHolly 5731370
Try automating scaling
MarcusHolly 22c3091
Test autoscaling function
MarcusHolly 98978a0
Merge branch 'main' into new_adm1_asm2d_interface
MarcusHolly 4159a92
Update AD initialization
MarcusHolly 22fd6a9
Delete new files and update old files
MarcusHolly 321b16e
Merge branch 'main' into new_adm1_asm2d_interface
MarcusHolly a205a7a
Add Chenyu's versions of solved flowsheets
MarcusHolly 87ccc2f
Clean up flowsheets
MarcusHolly 7150b66
Update test solutions
MarcusHolly b7b2c17
Minor cleanup
MarcusHolly d637ff4
Add scaling factor that was accidentally deleted
MarcusHolly 33b52e3
Add bac 2nd conservation test
MarcusHolly 9125cda
Merge branch 'main' into new_adm1_asm2d_interface
MarcusHolly 4617c92
Address some electroN-P issues
MarcusHolly c7b9d56
Add scaling function to BSM2 ui file
MarcusHolly a95b8e8
Update test files
MarcusHolly 07ffba0
address Pylint issue
MarcusHolly 9901f3d
Merge branch 'main' into new_adm1_asm2d_interface
MarcusHolly 0aaac45
Change from rel to abs
MarcusHolly b42f408
Merge branch 'main' into new_adm1_asm2d_interface
MarcusHolly 7d31abb
Change rel to abs for zero-values
MarcusHolly 0e93eb2
Try updating UI file
MarcusHolly 59dec65
Remove costing terms from extended BSM2 GUI
MarcusHolly 047b8c8
Merge branch 'main' into new_adm1_asm2d_interface
MarcusHolly e80d531
Remove bio_P config option
MarcusHolly dfc95ca
Address merge conflict
MarcusHolly 9c9655e
Chane one more rel to abs
MarcusHolly 814c527
Merge branch 'main' into new_adm1_asm2d_interface
MarcusHolly 4e3bc98
Comment out bio_P=False testing and use bio_P=True in UI
MarcusHolly 7b7aba8
Merge branch 'main' into new_adm1_asm2d_interface
MarcusHolly bf0bd1b
Add costing to ui
MarcusHolly d240381
Merge branch 'main' into new_adm1_asm2d_interface
MarcusHolly b64fba3
Address merge conflict
MarcusHolly 65794b6
Replace bio with X_{bio}
MarcusHolly 0e157dd
Delete unnecessary outputs from electroNP flowsheet
MarcusHolly 5674852
Correct mistake in ADM1-ASM2d translator
MarcusHolly 05de86b
Merge branch 'main' into new_adm1_asm2d_interface
lbianchi-lbl fa128e1
Flowsheet solving for bio_P = False
MarcusHolly 46872d3
Update both translator files based on Xavi feedback
MarcusHolly 5214c59
Merge branch 'new_adm1_asm2d_interface' of https://github.com/MarcusH…
MarcusHolly 81a0233
Undo unit changes in ASM2d/ADM1 translator
MarcusHolly f898cb6
Correct typo
MarcusHolly f8471ae
Update adm1_asm2d test
MarcusHolly c9eb9d2
Update documentation
MarcusHolly 1e6e114
Revised version of BSM2 and electroNP
luohezhiming 7ca83c3
Merge branch 'new_adm1_asm2d_interface' of https://github.com/MarcusH…
MarcusHolly 656fb38
Merge branch 'main' into new_adm1_asm2d_interface
MarcusHolly b469c9b
Clean up BSM2_P_extension flowsheet
MarcusHolly 230a4d3
Update BSM2 w/ P extension tests
MarcusHolly ffed59c
Update electroN-P flowsheet tests
MarcusHolly 3a1be89
Update BSM2-P GUI
MarcusHolly 3ea1f3c
Address pylint issue
MarcusHolly c719fbe
Delete tests for bio_P=True
MarcusHolly 46071a5
Have GUI use bio_P=False instead of bio_P=True
MarcusHolly c6700b5
Put common translator parameters inside of ADM1 reaction package
MarcusHolly 9bc5840
Use watertap solver instead of IDAES
MarcusHolly b3cd2bf
Update tests for bio_P=True and bio_P=False
MarcusHolly 95a6f9b
Add config option for bio_P into GUI
MarcusHolly f8476c2
Address pylint issues
MarcusHolly cbbfe6f
Try to resolve remaining test failures
MarcusHolly bb95f8d
Add watertap get_solver to test file
MarcusHolly cc3f243
Get rid of config option in BSM2-P GUI
MarcusHolly b63ce95
Try to resolve GUI issue
MarcusHolly 4dc04b3
Minor change to flowsheet scaling
MarcusHolly 4e8d9ed
Add require_idaes_solver to BSM2-P GUI
MarcusHolly 0729d39
Resolve solving issue
luohezhiming cc61553
Merge branch 'new_adm1_asm2d_interface' of https://github.com/MarcusH…
luohezhiming a7bdd21
Comment out everything related to costing
MarcusHolly 77bd768
Merge branch 'new_adm1_asm2d_interface' of https://github.com/MarcusH…
MarcusHolly c06fe0f
Address Pylint issues
MarcusHolly 3313322
Address Pylint issues
MarcusHolly b18c7f2
Merge branch 'main' into new_adm1_asm2d_interface
MarcusHolly 53baefd
Delete BSM2_electroNP flowsheet
MarcusHolly 88955db
Merge branch 'main' into new_adm1_asm2d_interface
MarcusHolly File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the 1 and 2 subscripts meant to represent (e.g., S_{ac, 1} or S_{ac, 2})?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - the 1 and 2 subscripts correspond to the concentration of the given component at steps 1 and 2 (named like
Sac_AD1
in the code). This translator only has 2 steps (whereas the other translator has 5).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A little bit confused here, e.g., I see you set
X_{PP,1} = 0
and then there are some conversion equations likeS_IP
will addX_{PP,1}
, but it is set as 0, so what is the purpose for this conversion? Add 0 does not seem to have any impacts of the outcome. and where doesX_PP
convert to?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have done that just to be explicit, and the c-code also leaves in such terms even when they are set equal to 0. I wanted to avoid somebody cross-checking our code with the c-code and wondering why a term is missing from an expression
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
X_PP is converted into S_Mg, S_K, and S_IP
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is correct. It has no effect since it's zero. But is it possible that in the future we want to change X_PP from 0 to something else? If so, I think it's worth keeping this zero term here in the documentation and in the code itself. It's also just the most accurate representation of these equations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adam-a-a hope you could also join the talk, I am confused because I believe X_PP should be important components of the system, right now, it seems no matter the initial value of X_PP is, it will directly set to 0 and no actual conversion happens to the system. Same conditions to X_PHA and X_PAO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh... yea there's a mistake here. S_{IP, 1} should be X_{PP, in) + ... and not S_{IP, in) + ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch @luohezhiming. This may or may not screw up the flowsheet again, but I'll test it out now...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will do a line-by-line review of the new translator file after this, which I just haven't had the time to do yet