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

add crystallizer flowsheet #1482

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

ElmiraShamlou
Copy link
Contributor

@ElmiraShamlou ElmiraShamlou commented Aug 20, 2024

add crystallizer flowsheet

Legal Acknowledgement

By contributing to this software project, I agree to the following terms and conditions for my contribution:

  1. I agree my contributions are submitted under the license terms described in the LICENSE.txt file at the top level of this directory.
  2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

@ElmiraShamlou ElmiraShamlou marked this pull request as draft August 21, 2024 12:33
Comment on lines +104 to +105
m.fs.properties = props.NaClParameterBlock()
m.fs.properties_nacl = props_nacl.NaClParameterBlock()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why use two separate NaCl property models? Any reason not to use NaCl with temp dependence by itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The key difference between the two property packages is that the crystallizer-specific package includes additional solid and vapor phases, which are not present in the other unit models.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense--I should've double-checked the imports.

from idaes.core import UnitModelCostingBlock

from watertap.property_models.unit_specific import cryst_prop_pack as props
from watertap.unit_models.Crystallizer_revised import Crystallization
Copy link
Contributor

Choose a reason for hiding this comment

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

I am interested to see how the revised form differs from the original. For one, I would prefer to have the revised version condensed within the original, accessible through config options, assuming there is considerable overlap in code. Waiting to see this unit model file to determine that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose you can get rid of Crystallizer_revised and just use the revised original now, right?

Copy link
Contributor

@adam-a-a adam-a-a left a comment

Choose a reason for hiding this comment

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

Here are some initial comments from a first pass

@@ -93,7 +93,7 @@ def build_crystallizer_cost_param_block(blk):
)

costing = blk.parent_block()
costing.register_flow_type("steam", blk.steam_cost)
#costing.register_flow_type("steam", blk.steam_cost)
Copy link
Contributor

Choose a reason for hiding this comment

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

delete commented code

Comment on lines -123 to -143
blk.costing_package.cost_flow(
pyo.units.convert(
(
blk.unit_model.magma_circulation_flow_vol
* blk.unit_model.dens_mass_slurry
* Constants.acceleration_gravity
* blk.costing_package.crystallizer.pump_head_height
/ blk.costing_package.crystallizer.efficiency_pump
),
to_units=pyo.units.kW,
),
"electricity",
)

blk.costing_package.cost_flow(
pyo.units.convert(
(blk.unit_model.work_mechanical[0] / _compute_steam_properties(blk)),
to_units=pyo.units.m**3 / pyo.units.s,
),
"steam",
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious about why these lines are being deleted. I probably didn't look closely enough, but have they been moved elsewhere or completely removed? And why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adam-a-a They are removed as we integrate the steam heater, pump, and other essential components into the full FC crystallizer flowsheet. Therefore, the flow rates of the steam and recycle streams, along with their associated costs, are incorporated within the cost calculations of the steam heater and pump unit models.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ElmiraShamlou but removing this hinders whoever wants to use the combined unit we had previously.

Maybe what we need is a configuration argument that determines the mode the user wants (broken down or combined), and then adds the necessary equations for each case?

@@ -215,72 +194,3 @@ def cost_crystallizer_by_volume(blk):
)
)
_cost_crystallizer_flows(blk)


def _compute_steam_properties(blk):
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question here, except it makes more sense immediately as to why we wouldn't want/need this in a costing module

Comment on lines +104 to +105
m.fs.properties = props.NaClParameterBlock()
m.fs.properties_nacl = props_nacl.NaClParameterBlock()
Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense--I should've double-checked the imports.

m.fs.distillate = Product(property_package= m.fs.properties_vapor)


# unit models: steam heater, mixer, pump, crystalizer, compressor, separator
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# unit models: steam heater, mixer, pump, crystalizer, compressor, separator
# unit models: steam heater, mixer, pump, crystallizer, compressor, separator

)

"""
self.work_mechanical = Var(
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm surprised to see no tests fail for the crystallizer unit especially since no changes were made to the test file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The tests are failing, and I'm revising them to align with the recent changes.


optimize_set_up(m)

#interval_initializer(m)
Copy link
Contributor

Choose a reason for hiding this comment

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

I am guessing that an InfeasibleConstraint exception (or something like that) arises when using the interval initializer.

@@ -0,0 +1,508 @@
#################################################################################
Copy link
Contributor

Choose a reason for hiding this comment

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

Next step is to add (1) a test file for this flowsheet and (2) documentation for this flowsheet in this section: https://watertap.readthedocs.io/en/latest/technical_reference/flowsheets/index.html

@@ -0,0 +1,444 @@
#################################################################################
Copy link
Contributor

Choose a reason for hiding this comment

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

Next step is to add (1) a test file for this flowsheet and (2) documentation for this flowsheet in this section: https://watertap.readthedocs.io/en/latest/technical_reference/flowsheets/index.html

Comment on lines +74 to +75
#from watertap.core.util.model_debug_mode import activate
#activate()
Copy link
Contributor

Choose a reason for hiding this comment

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

delete

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:Normal Normal Priority Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants