Add MD 0D #1105

ElmiraShamlou · 2023-08-16T18:56:40Z

Fixes/Resolves:

(replace this with the issue # fixed or resolved, if no issue exists then a brief statement of what this PR does)

Summary/Motivation:

Changes proposed in this PR:

Legal Acknowledgement

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

I agree my contributions are submitted under the license terms described in the LICENSE.txt file at the top level of this directory.
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.

…to new_MD

codecov · 2023-08-17T03:26:14Z

Codecov Report

Attention: 39 lines in your changes are missing coverage. Please review.

Comparison is base (164a775) 94.88% compared to head (be3aa17) 94.86%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1105      +/-   ##
==========================================
- Coverage   94.88%   94.86%   -0.02%     
==========================================
  Files         336      340       +4     
  Lines       33269    33931     +662     
==========================================
+ Hits        31566    32189     +623     
- Misses       1703     1742      +39

Files	Coverage Δ
...atertap/unit_models/MD/membrane_distillation_0D.py	`98.64% <98.64%> (ø)`
watertap/unit_models/MD/MD_channel_0D.py	`94.80% <94.80%> (ø)`
watertap/unit_models/MD/MD_channel_base.py	`94.32% <94.32%> (ø)`
...ertap/unit_models/MD/membrane_distillation_base.py	`92.13% <92.13%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

adam-a-a · 2023-08-25T21:20:44Z

watertap/unit_models/MD/MD_channel_0D.py

+        if type == "hot_ch":
+            state_args_properties_out = state_args["hot_outlet"]
+
+            self.properties_out.initialize(
+                state_args=state_args_properties_out,
+                outlvl=outlvl,
+                optarg=optarg,
+                solver=solver,
+            )
+        elif type == "cold_ch":
+            state_args_properties_out = state_args["cold_outlet"]
+
+            self.properties_out.initialize(
+                state_args=state_args_properties_out,
+                outlvl=outlvl,
+                optarg=optarg,
+                solver=solver,
+            )
+        else:
+            raise ConfigurationError(
+                "Either hot_ch or cold_ch must be set in the configuration."
+            )


Why is it one or the other? Shouldn't both be initialized? Also, it looks like you would need pass strings to type here, which I don't think we want to do.

cold outlet and hot outlet are initialized differently based on inlet

adam-a-a · 2023-08-25T21:25:30Z

watertap/unit_models/MD/membrane_distillation_0D.py

+    CONFIG.declare(
+        "dynamic",
+        ConfigValue(
+            default=False,
+            domain=In([False]),
+            description="Dynamic model flag - must be False",
+            doc="""Indicates whether this model will be dynamic or not.
+    **default** - False. Membrane units do not yet support dynamic
+    behavior.""",
+        ),
+    )
+    CONFIG.declare(
+        "has_holdup",
+        ConfigValue(
+            default=False,
+            domain=In([False]),
+            description="Holdup construction flag - must be False",
+            doc="""Indicates whether holdup terms should be constructed or not.
+    **default** - False. Membrane units do not have defined volume, thus
+    this must be False.""",
+        ),
+    )


Any reason that we need dynamic and has_holdup in the main config block as well as in the config template for hot and cold channels? I would think you could eliminate these 2 from config_template, but maybe there is some reason for doing this.

adam-a-a · 2023-08-25T21:27:11Z

watertap/unit_models/MD/membrane_distillation_0D.py

+    _CONFIG_Template.declare(
+        "property_package_vapor",
+        ConfigValue(
+            default=useDefault,
+            domain=is_physical_parameter_block,
+            description="Property package to use for control volume",
+            doc="""Property parameter object used to define property calculations,
+        **default** - useDefault.
+        **Valid values:** {
+        **useDefault** - use default package from parent model or flowsheet,
+        **PhysicalParameterObject** - a PhysicalParameterBlock object.}""",
+        ),
+    )
+    _CONFIG_Template.declare(
+        "property_package_args_vapor",
+        ConfigBlock(
+            implicit=True,
+            description="Arguments to use for constructing property packages",
+            doc="""A ConfigBlock with arguments to be passed to a property block(s)
+        and used when constructing these,
+        **default** - None.
+        **Valid values:** {
+        see property package for documentation.}""",
+        ),
+    )
+


Is there any need to have property_package_vapor specified for each channel, or can we just specify it once on the main CONFIG block?

From the tests, I see you need to specify twice (once for each channel). Maybe there is some alternative configuration where you might want to consider 2 different vapor packages that I am not seeing, but from my current perspective, I don't see any immediate use cases for having 2 different vapor property packages.

adam-a-a · 2023-08-25T21:50:52Z

watertap/unit_models/MD/membrane_distillation_base.py

+        @self.Constraint(
+            self.flowsheet().config.time,
+            self.difference_elements,
+            doc="hot side evaporation enthalpy flux",
+        )
+        def eq_flux_hot_enth(b, t, x):
+            return (
+                b.flux_enth_hot[t, x]
+                == b.flux_mass[t, x]
+                * b.hot_ch.properties_vapor[t, x].enth_mass_phase["Vap"]
+            )
+
+        @self.Constraint(
+            self.flowsheet().config.time,
+            self.difference_elements,
+            doc="cold side evaporation enthalpy flux",
+        )
+        def eq_flux_cold_enth(b, t, x):
+            return (
+                b.flux_enth_cold[t, x]
+                == b.flux_mass[t, x]
+                * b.cold_ch.properties_vapor[t, x].enth_mass_phase["Vap"]
+            )
+
+        @self.Expression(
+            self.flowsheet().config.time,
+            doc="Average hot side enthalpy flux expression",
+        )
+        def flux_enth_hot_avg(b, t):
+            return (
+                sum(b.flux_enth_hot[t, x] for x in self.difference_elements) / self.nfe
+            )
+
+        @self.Expression(
+            self.flowsheet().config.time,
+            doc="Average cold side enthalpy flux expression",
+        )
+        def flux_enth_cold_avg(b, t):
+            return (
+                sum(b.flux_enth_cold[t, x] for x in self.difference_elements) / self.nfe
+            )


Shouldn't these be consolidated and put in MD_channel_base (or MD_channel_0D)?

Mass and heat fluxes connect the channels but are not defined for each individual channel. Even the eq_flux_hot_enth and eq_flux_cold_enth are defined by the mass flux that connects the hot and cold channels.

adam-a-a · 2023-08-25T21:52:34Z

watertap/unit_models/MD/membrane_distillation_base.py

+        self.flux_enth_hot = Var(
+            self.flowsheet().config.time,
+            self.difference_elements,
+            initialize=1e3,
+            bounds=(1e-4, 1e5),
+            units=pyunits.J * pyunits.s**-1 * pyunits.m**-2,
+            doc="hot side evaporation enthalpy flux",
+        )
+
+        self.flux_enth_cold = Var(
+            self.flowsheet().config.time,
+            self.difference_elements,
+            initialize=1e3,
+            bounds=(1e-4, 1e5),
+            units=pyunits.J * pyunits.s**-1 * pyunits.m**-2,
+            doc="cold side condensation enthalpy flux",
+        )


Can't these go in one of the channel files? Then, you could later reference hot_ch.flux_enth or cold_ch.flux_enth

Only these two variables, but not their associated constraints, may go to MD_channel_base.

adam-a-a · 2023-08-25T21:54:42Z

watertap/unit_models/MD/membrane_distillation_0D.py

+    CONFIG.declare("hot_ch", _CONFIG_Template(doc="hot channel config arguments"))
+    CONFIG.declare("cold_ch", _CONFIG_Template(doc="cold channel config arguments"))


Minor preference here, but I think it'd be better to fully spell out hot_channel and cold_channel.

We can do this in future PRs as it requires changing many lines of code.

…to new_MD

lbianchi-lbl · 2023-08-31T20:52:35Z

@ElmiraShamlou is planning to work on some details and documentation but overall should be soon ready for review.

…ts, add documentation

TimBartholomew

I primarily reviewed the documentation and test file and it looks good.

One suggestion I have that would be for a later PR would be to change "ch" to "channel". Renaming won't be that big of deal, but lots of lines of code would change.

savannahsakhai

I attempted to look through all of the files. It looks good to me.

I made comments on a few possible typos.

savannahsakhai · 2023-09-28T19:40:14Z

watertap/unit_models/MD/MD_channel_0D.py

+    ):
+        super().add_state_blocks(has_phase_equilibrium=has_phase_equilibrium)
+
+        # quack like a 1D model


What is meant here? I think there is a typo.

@savannahsakhai Even though the model is 0D, it intentionally includes a length domain and inlet/outlet properties, aligning with the flow direction, much like a 1D model would

savannahsakhai · 2023-09-28T19:45:46Z

watertap/unit_models/MD/MD_channel_base.py

+            solute_set,
+            doc="Concentration polarization modulus",
+        )
+        def eq_cp_modulus(b, t, x, j):  # pylint: disable=function-redefined


Was this comment meant to be left in?

savannahsakhai · 2023-09-28T19:50:08Z

watertap/unit_models/MD/membrane_distillation_base.py

+            # self.config.hot_ch.property_package.phase_list,
+            # self.config.hot_ch.property_package.component_list,


Are these meant to be left this way? Are they serving a purpose?

We typically delete commented code, so we can delete in a subsequent PR.

savannahsakhai · 2023-09-28T20:05:49Z

watertap/unit_models/MD/membrane_distillation_base.py

+        self.flux_enth_hot = Var(
+            self.flowsheet().config.time,
+            self.difference_elements,
+            initialize=1e3,
+            bounds=(1e-10, 1e5),
+            units=pyunits.J * pyunits.s**-1 * pyunits.m**-2,
+            doc="hot side evaporation enthalpy flux",
+        )
+
+        self.flux_enth_cold = Var(
+            self.flowsheet().config.time,
+            self.difference_elements,
+            initialize=1e3,
+            bounds=(1e-10, 1e5),
+            units=pyunits.J * pyunits.s**-1 * pyunits.m**-2,
+            doc="cold side condensation enthalpy flux",
+        )


The variables directly above this use units_meta. Is there a reason that these variables do not follow the same convention? I believe this only occurs a few times across the files.

savannahsakhai · 2023-09-28T20:07:34Z

watertap/unit_models/tests/test_membrane_distillation_0D.py

+        m.fs.unit.permeability_coef.fix(1e-10)
+        m.fs.unit.membrane_thickness.fix(1e-4)
+        m.fs.unit.membrane_tc.fix(0.2)
+        # m.fs.unit.hot_ch.cp_modulus.fix(1)


Is this comment meant to be left this way?

adam-a-a

LGTM - @ElmiraShamlou please create an issue tracking any comments here that we would consider in a follow-up PR.

ElmiraShamlou and others added 22 commits May 17, 2023 09:26

added MD_0D model

d34c0a5

removed unused imports

daa426f

removed unused import

561429c

Merge branch 'main' into MD

16d4a33

modifies constraints

2665160

Merge branch 'MD' of https://github.com/ElmiraShamlou/watertap into MD

ea0803b

Merge branch 'main' into MD

a31fe60

removes unused imports

a2b8fb2

Merge branch 'MD' of https://github.com/ElmiraShamlou/watertap into MD

d9dda26

minor modifications

b923d3f

Merge branch 'main' into MD

9f45415

Merge branch 'main' into MD

675f68c

Merge branch 'main' into MD

47f19fe

changes MD to utilize waterTAP core functions

29c8767

Merge branch 'main' into MD

a1fb423

Merge branch 'MD' of https://github.com/ElmiraShamlou/watertap into MD

a6eb214

add necessary files to use watertap membrane channel

9074bf4

changes to the MD0D

dbc06ca

add MD model

45fc3e5

add membrane distillation 0D

f63732e

delete unncessary files

bfdf0ba

removes previous MD files

2cf71b4

adam-a-a marked this pull request as draft August 16, 2023 23:03

ElmiraShamlou and others added 3 commits August 16, 2023 21:44

Merge branch 'main' into new_MD

450cb34

black

8c1ca92

Merge branch 'new_MD' of https://github.com/ElmiraShamlou/watertap in…

8c1e6fe

…to new_MD

removes unused files

ceed06c

ksbeattie added the Priority:Normal Normal Priority Issue or PR label Aug 17, 2023

ksbeattie assigned ElmiraShamlou Aug 17, 2023

adam-a-a reviewed Aug 25, 2023

View reviewed changes

ElmiraShamlou added 10 commits August 28, 2023 13:07

modified test

b4c3a6e

test modification

1978ca0

Merge branch 'new_MD' of https://github.com/ElmiraShamlou/watertap in…

5fdc3e9

…to new_MD

add init

b589830

modifies imports

0d977c4

removes unused imports

a32a170

removes unsused imports

b218ef9

black

98bc6f6

improves convective heat scaling

3bcaeec

additional tests for various configurations

f5b1528

ElmiraShamlou added 2 commits September 20, 2023 07:30

add thermal performance metrics, improves initialization, updates tes…

0ec826b

…ts, add documentation

removes unused import

cb3288e

ElmiraShamlou marked this pull request as ready for review September 20, 2023 12:03

ElmiraShamlou and others added 2 commits September 20, 2023 08:33

Merge branch 'main' into new_MD

93c918a

modifies documentation

9d3cbec

TimBartholomew approved these changes Sep 22, 2023

View reviewed changes

savannahsakhai approved these changes Sep 28, 2023

View reviewed changes

adam-a-a approved these changes Sep 28, 2023

View reviewed changes

ksbeattie added Priority:High High Priority Issue or PR and removed Priority:Normal Normal Priority Issue or PR labels Sep 28, 2023

Merge branch 'main' into new_MD

be3aa17

lbianchi-lbl enabled auto-merge (squash) September 29, 2023 01:13

lbianchi-lbl merged commit a40bd78 into watertap-org:main Sep 29, 2023
24 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add MD 0D #1105

Add MD 0D #1105

ElmiraShamlou commented Aug 16, 2023

codecov bot commented Aug 17, 2023 •

edited

Loading

adam-a-a Aug 25, 2023

ElmiraShamlou Aug 25, 2023

adam-a-a Aug 25, 2023

adam-a-a Aug 25, 2023

adam-a-a Aug 25, 2023

adam-a-a Aug 25, 2023

ElmiraShamlou Sep 27, 2023

adam-a-a Aug 25, 2023

ElmiraShamlou Sep 27, 2023

adam-a-a Aug 25, 2023

ElmiraShamlou Sep 27, 2023

lbianchi-lbl commented Aug 31, 2023

TimBartholomew left a comment

savannahsakhai left a comment

savannahsakhai Sep 28, 2023

ElmiraShamlou Sep 28, 2023 •

edited

Loading

savannahsakhai Sep 28, 2023

savannahsakhai Sep 28, 2023

adam-a-a Sep 28, 2023

savannahsakhai Sep 28, 2023 •

edited

Loading

savannahsakhai Sep 28, 2023

adam-a-a left a comment

		CONFIG.declare("hot_ch", _CONFIG_Template(doc="hot channel config arguments"))
		CONFIG.declare("cold_ch", _CONFIG_Template(doc="cold channel config arguments"))

		# self.config.hot_ch.property_package.phase_list,
		# self.config.hot_ch.property_package.component_list,

Add MD 0D #1105

Add MD 0D #1105

Conversation

ElmiraShamlou commented Aug 16, 2023

Fixes/Resolves:

Summary/Motivation:

Changes proposed in this PR:

Legal Acknowledgement

codecov bot commented Aug 17, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbianchi-lbl commented Aug 31, 2023

TimBartholomew left a comment

Choose a reason for hiding this comment

savannahsakhai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElmiraShamlou Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

savannahsakhai Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adam-a-a left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 17, 2023 •

edited

Loading

ElmiraShamlou Sep 28, 2023 •

edited

Loading

savannahsakhai Sep 28, 2023 •

edited

Loading