-
Notifications
You must be signed in to change notification settings - Fork 238
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
Adding StreamScaler unit model, along with the test and rst file. #1517
Conversation
…to build.py timing out
… stream_scaler_Polley # Conflicts: # idaes/models/unit_models/tests/test_stream_scaler.py
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 few comments fro ma first pass - the main thing is that the docs need to tell the user more about what this model does.
However, I think we should consider a different name for this model as including Scaler
in the name will probably cause confusion with the new Scaler
classes for numerical scaling.
docs/reference_guides/model_libraries/generic/unit_models/stream_scaler.rst
Outdated
Show resolved
Hide resolved
docs/reference_guides/model_libraries/generic/unit_models/stream_scaler.rst
Show resolved
Hide resolved
docs/reference_guides/model_libraries/generic/unit_models/stream_scaler.rst
Outdated
Show resolved
Hide resolved
docs/reference_guides/model_libraries/generic/unit_models/stream_scaler.rst
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1517 +/- ##
==========================================
- Coverage 77.06% 77.06% -0.01%
==========================================
Files 386 387 +1
Lines 62418 62486 +68
Branches 10230 10236 +6
==========================================
+ Hits 48101 48153 +52
- Misses 11884 11900 +16
Partials 2433 2433 ☔ View full report in Codecov by Sentry. |
… no solve or numerical tests to be done for the iapws since the unit model has no unit model level contraints. Tried to run the test_solve but got no feasible solution
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.
Looks good 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.
Besides my minor comments below, I think a test with a multiplier value greater than 1 would be useful to confirm the unit behavior. For example, if feed has a flow of 1 and the multiplier value is 2, does the outlet stream have a flow of 0.5 or is just the scaling factor 0.5? Also a check on a multiplier value of 0, whether values less than 1 should be allowed, or if only integer values should be allowed.
Fixes
Adding a python file that contains a new unit model "StreamScaler" along with a test file and a rst file.
Summary/Motivation:
Adding a useful unit model build by Douglas Allen. This pull request was a task for me to learn and practice the skills necessary to become proficient and code/software management
Changes proposed in this PR:
build.py
file when building the docs. It always times out for me at the default 180 secondsLegal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: