add MD1D #1323
add MD1D #1323
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1323 +/- ##
==========================================
+ Coverage 93.73% 93.80% +0.07%
==========================================
Files 337 339 +2
Lines 35328 35512 +184
==========================================
+ Hits 33114 33313 +199
+ Misses 2214 2199 -15 ☔ View full report in Codecov by Sentry. |
| ) | ||
|
|
||
| else: | ||
| b.cold_ch.mass_transfer_term[t, p, j].fix(0) |
There was a problem hiding this comment.
How is hot_ch.mass_transfer_term handled for vapor?
There was a problem hiding this comment.
Is it essentially just a stale variable?
There was a problem hiding this comment.
@adam-a-a The hot channel does not include a mass_transfer_term for the vapor phase because it uses solution property packages that exclude vapor phase properties. Instead, the vapor state block is managed independently using a separate vapor property package, and it does not affect the main state blocks of the channel control volume
| ) | ||
| else: | ||
| b.cold_ch.mass_transfer_term[t, p, j].fix(0) | ||
| b.hot_ch.mass_transfer_term[t, p, j].fix(0) |
There was a problem hiding this comment.
Ah, I guess this is where you handle hot_ch.mass_transfer_term.
There was a problem hiding this comment.
@adam-a-a This condition ensures that solutes do not cross the membrane (assumes there is no membrane wetting).
| mass_transfer_coefficient=MassTransferCoefficient.none, | ||
| ) | ||
| if hasattr(self.cold_ch.config.property_package, "solute_set"): | ||
| # If solute_set is defined, add concentration polarization configuration |
There was a problem hiding this comment.
Is this necessary because of the vapor state block?
There was a problem hiding this comment.
@adam-a-a Some of the changes in this PR, including this one, lay the groundwork for supporting various MD configurations like VMD (upcoming PRs), ensuring channels can adapt to various property packages including pure water. This specific code line addresses scenarios with pure water, where concentration and concentration polarization do not apply.
| CONFIG.declare( | ||
| "finite_elements", | ||
| ConfigValue( | ||
| default=20, |
There was a problem hiding this comment.
Any reason for bumping this up to 20?
There was a problem hiding this comment.
@adam-a-a Based on previous MD model experiences, changes in results are negligible beyond this point.
There was a problem hiding this comment.
That makes sense--although I still question whether the default needs to be that value as opposed to a lower value that would solve quicker. I would say if the time to solve isn't too lengthy (for exploratory purposes) at 20 elements, then this is fine. I could see the case where we might want to reduce this value for some tests that will constantly rerun with every PR.
Whatever the case may be, we can save this for a future PR if we think it would be best to make any adjustments (either in tests or directly to default).
|
Should the documentation for 0D be updated to match subtle changes here? |
Co-authored-by: Adam Atia <aatia@keylogic.com>
@adam-a-a I believe the core process principles remain unchanged in this PR, so there's no need to update the documentation for 0D. The changes primarily address how the model interacts with different property packages, setting the stage for future MD configurations. However, I need to include documentation for MD 1D, which will be very similar to MD 0D. |
|
@ElmiraShamlou could you also create an issue to track any potential items for the future? For example, 1D documentation, future configs, etc. ? |
Add Membrane distillation 1D
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: