Skip to content

Conversation

@ElmiraShamlou
Copy link
Contributor

@ElmiraShamlou ElmiraShamlou commented Aug 20, 2024

add three crystallizer flowsheets

  1. MVC with crystallizer
  2. Crystallizer with chiller and condenser (no heat recovery)
  3. TVC with crystallizer

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
@ksbeattie ksbeattie added the Priority:Normal Normal Priority Issue or PR label Aug 22, 2024
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


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?

_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

Copy link
Contributor

Choose a reason for hiding this comment

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

this should really be part of the property package

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

@adam-a-a adam-a-a added the 🍒 Merge to rel Merge commit(s) from this PR to release branch label Dec 11, 2024
@lbianchi-lbl lbianchi-lbl removed the 🍒 Merge to rel Merge commit(s) from this PR to release branch label Dec 18, 2024
@ksbeattie
Copy link
Contributor

This is waiting on NAWI funds for work to continue.

@ksbeattie
Copy link
Contributor

@ElmiraShamlou and this one too?

@kurbansitterley
Copy link
Contributor

I made these changes initially because the flowsheet wasn't working and I have a student that wants to use it. But also my changes match our conventions.

I reverted @adam-a-a 's modifications for steam costing where steam_cost was on the main costing block (e.g., m.fs.costing). I don't think we want to cost steam this way. Instead, models that use steam have their own build_steam_cost_param_block that results in the steam cost being at m.fs.costing.steam.cost. This matches how we cost materials for other units (though I just checked and it appears electroNP uses the old convention).

In making these changes, I discovered that we have two models that use a steam cost with different units: heat_exchanger uses $/kg but surrogate_crystallizer uses $/m3. Right now, if these two units were on the same flowsheet, one or the other would run into errors. This could be fixed by either changing the expression used to cost the steam flow or have two different steam costs e.g., steam_per_m3 and steam_per_kg. Note that this would still have been the case if I hadn't made these changes.

Crystallizer tests are failing but that was the case prior to my changes.

@adam-a-a
Copy link
Contributor

adam-a-a commented Oct 1, 2025

I made these changes initially because the flowsheet wasn't working and I have a student that wants to use it. But also my changes match our conventions.

I reverted @adam-a-a 's modifications for steam costing where steam_cost was on the main costing block (e.g., m.fs.costing). I don't think we want to cost steam this way. Instead, models that use steam have their own build_steam_cost_param_block that results in the steam cost being at m.fs.costing.steam.cost. This matches how we cost materials for other units (though I just checked and it appears electroNP uses the old convention).

In making these changes, I discovered that we have two models that use a steam cost with different units: heat_exchanger uses $/kg but surrogate_crystallizer uses $/m3. Right now, if these two units were on the same flowsheet, one or the other would run into errors. This could be fixed by either changing the expression used to cost the steam flow or have two different steam costs e.g., steam_per_m3 and steam_per_kg. Note that this would still have been the case if I hadn't made these changes.

Crystallizer tests are failing but that was the case prior to my changes.

@kurbansitterley my main question here is what will currently happen when more than one unit on the flowsheet has its own build_steam_cost_param_block? What protects the user from unintentionally having different steam costs for different units when there should be a single, global steam cost (not considering the case where the user might want more than one steam cost on a flowsheet)?

@kurbansitterley
Copy link
Contributor

I made these changes initially because the flowsheet wasn't working and I have a student that wants to use it. But also my changes match our conventions.
I reverted @adam-a-a 's modifications for steam costing where steam_cost was on the main costing block (e.g., m.fs.costing). I don't think we want to cost steam this way. Instead, models that use steam have their own build_steam_cost_param_block that results in the steam cost being at m.fs.costing.steam.cost. This matches how we cost materials for other units (though I just checked and it appears electroNP uses the old convention).
In making these changes, I discovered that we have two models that use a steam cost with different units: heat_exchanger uses $/kg but surrogate_crystallizer uses $/m3. Right now, if these two units were on the same flowsheet, one or the other would run into errors. This could be fixed by either changing the expression used to cost the steam flow or have two different steam costs e.g., steam_per_m3 and steam_per_kg. Note that this would still have been the case if I hadn't made these changes.
Crystallizer tests are failing but that was the case prior to my changes.

@kurbansitterley my main question here is what will currently happen when more than one unit on the flowsheet has its own build_steam_cost_param_block? What protects the user from unintentionally having different steam costs for different units when there should be a single, global steam cost (not considering the case where the user might want more than one steam cost on a flowsheet)?

My understanding is that register_costing_parameter_block will make this check and only create a new block if one doesn't exist for steam. I believe a critical assumption here is that the material is spelled identically. i.e., nacl and NaCl would result in two parameter blocks for NaCl. In the scenario where two units both use steam with two different costs, the cost used in the costing package would be that of the first unit costing model instantiated. Of course this could be modified at the flowsheet level with e.g., m.fs.costing.steam.cost.fix(value)

@kurbansitterley
Copy link
Contributor

@ElmiraShamlou added tests for the new flowsheets; only testing main

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.

6 participants