-
Notifications
You must be signed in to change notification settings - Fork 57
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
Update nf scaling. #1145
Update nf scaling. #1145
Changes from 14 commits
14b0274
e8d8df2
6339842
498cda2
7dbe1ca
1ea8928
9e2615f
a8ec4c7
c8f8315
50d5de4
17e4d18
54fefd1
2c4d4aa
740c772
5e8f95d
be85e4f
6cc035b
17291e3
cd7e6e7
608af7a
fbc8617
c647ea9
1b599d1
02f00db
281a992
1644dfc
5ba4974
6ea0e3e
ce6f865
7857c71
8771da9
18c5574
4b6ebae
f89e036
b1fea16
0e22f60
5918800
3be712a
ee46fb6
632d997
9df6623
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,13 +20,13 @@ | |
def test_main(): | ||
m = main() | ||
test_dict = { | ||
"lcow": [m.fs.costing.LCOW, 0.144583], | ||
"pressure": [m.fs.NF.pump.outlet.pressure[0] * 1e-5, 5.372779], | ||
"area": [m.fs.NF.nfUnit.area, 405.66782957], | ||
"lcow": [m.fs.costing.LCOW, 0.141678], | ||
"pressure": [m.fs.NF.pump.outlet.pressure[0] * 1e-5, 5.21856], | ||
"area": [m.fs.NF.nfUnit.area, 444.423], | ||
"recovery": [ | ||
m.fs.NF.nfUnit.recovery_vol_phase[0.0, "Liq"] * 100, | ||
90, | ||
94.64, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My only concern about letting recovery be "optimized" is that it'll drive up to the upper bound and at least previously, the test solution will deviate by a little every now and then with new changes in future PRs. It may be the case that the latest changes to scaling have remedied the issue completely, but it might also be the case that we'll see future tests fail because the solution converged to another recovery value close to 95% upper bound but outside of the tolerance of the regression test. |
||
], | ||
} | ||
for (model_result, testval) in test_dict.values(): | ||
for model_result, testval in test_dict.values(): | ||
assert pytest.approx(testval, rel=1e-3) == value(model_result) |
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.
Does this still break if you fix to 90% after scale refinement (considering all the latest changes with all checks passing)?
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 still would prefer fixing recovery rate here simply for the sake of avoiding future test failures due to recov rate riding up to the upper bound and subtle variations that could occur from that, but going to approve this anyway and leave the choice to you