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

AirStripping0D and AirWaterEquilibrium Property Model + Docs #85

Merged
merged 131 commits into from
Jan 10, 2024

Conversation

kurbansitterley
Copy link
Contributor

@kurbansitterley kurbansitterley commented Nov 30, 2023

This PR will add:

  • AirWaterEquilibrium (AWE) property model and related tests
  • Docs for AWE
  • AirStripping0D unit model and related tests
  • Docs for air stripping model
  • Docs for basic water property package

@kurbansitterley kurbansitterley added the help wanted Extra attention is needed label Nov 30, 2023
@kurbansitterley kurbansitterley self-assigned this Nov 30, 2023
Comment on lines +1289 to +1329
self.collision_function_zeta_param_A = a = Param(
initialize=-0.14329,
units=pyunits.dimensionless,
doc="Collision function zeta equation - A parameter",
)

self.collision_function_zeta_param_B = b_ = Param(
initialize=-0.48343,
units=pyunits.dimensionless,
doc="Collision function zeta equation - B parameter",
)

self.collision_function_zeta_param_C = c = Param(
initialize=0.1939,
units=pyunits.dimensionless,
doc="Collision function zeta equation - C parameter",
)

self.collision_function_zeta_param_D = d = Param(
initialize=0.13612,
units=pyunits.dimensionless,
doc="Collision function zeta equation - D parameter",
)

self.collision_function_zeta_param_E = e = Param(
initialize=-0.20578,
units=pyunits.dimensionless,
doc="Collision function zeta equation - E parameter",
)

self.collision_function_zeta_param_F = f = Param(
initialize=0.083899,
units=pyunits.dimensionless,
doc="Collision function zeta equation - F parameter",
)

self.collision_function_zeta_param_G = g = Param(
initialize=-0.011491,
units=pyunits.dimensionless,
doc="Collision function zeta equation - G parameter",
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Conventionally, I think these params would typically go on the param block. Any particular reason why you did it this way, other than convenience while developing?

I suppose the only reason it matters is that one would expect to access parameters from the param block, and that'd be pretty consistent with most things in IDAES/WaterTAP, if my recollection is correct.

Copy link
Contributor

@adam-a-a adam-a-a Dec 5, 2023

Choose a reason for hiding this comment

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

I guess the other advantage of doing it this way is that parameters associated with on-demand props wouldn't be built unnecessarily, and building these on the param block would be less straight-forward if you wanted to also build on-demand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both of those reasons are pretty much it.

And I suppose I'd push back a bit on this convention. I don't see the advantage/logic behind putting them on the param block and them always being built, but maybe I am missing something. It makes more sense to me to build property-specific params when the on-demand property is built. I would liken this to how we started putting unit-specific costing parameters only in unit-specific costing packages so they weren't always built and cluttering up calls to .display() or sifting through components via .component_objects() calls (maybe I am the only one that does that regularly though...)

But I definitely won't die on this hill. I am not opposed to putting them on the param block (or making them Vars, which appears to also be convention).

Copy link
Contributor

Choose a reason for hiding this comment

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

I won't die on this hill either. There are mountains to climb! =)

@kurbansitterley kurbansitterley changed the title AirStripping0D and AirWaterEquilibrium Property Model AirStripping0D and AirWaterEquilibrium Property Model + Docs Jan 2, 2024
@kurbansitterley
Copy link
Contributor Author

@adam-a-a do you mind doing a review of the documentation (added since your last review)

Comment on lines +73 to +74
Sets
----
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you intend to add sets here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops just added

Comment on lines 269 to 270
doc="Safety factor for tower height",
doc="Factor to calculate tower height",
Copy link
Contributor

@adam-a-a adam-a-a Jan 10, 2024

Choose a reason for hiding this comment

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

Minor, but any reason why you moved away from calling it "safety factor"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it isn't a safety factor. It is just the factor used to calculate the tower height from packing height. i.e., tower_height = packing_height * tower_height_factor

Seemed like a misnomer to me.

Copy link
Collaborator

@zacharybinger zacharybinger left a comment

Choose a reason for hiding this comment

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

Small comments, but overall approved

@kurbansitterley kurbansitterley merged commit fee5aaa into watertap-org:main Jan 10, 2024
@kurbansitterley kurbansitterley deleted the air_stripping branch December 10, 2024 00:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants