Skip to content

Knook/multicomponent #4322

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

Open
wants to merge 35 commits into
base: release
Choose a base branch
from
Open

Knook/multicomponent #4322

wants to merge 35 commits into from

Conversation

KarsKnook
Copy link
Contributor

Demo for multicomponent flows -- microfluidic mixing of hydrocarbons

@KarsKnook KarsKnook changed the base branch from master to release May 14, 2025 10:51
@KarsKnook KarsKnook marked this pull request as draft May 14, 2025 10:54
@KarsKnook KarsKnook requested a review from pefarrell May 20, 2025 16:46
@KarsKnook
Copy link
Contributor Author

The references are not showing using make html?

KarsKnook and others added 5 commits May 20, 2025 18:42
* Small suggestions

* Don't mention Gibbs-Duhem without explaining later

* Change initial paragraph
@KarsKnook KarsKnook marked this pull request as ready for review May 21, 2025 12:43
@KarsKnook KarsKnook requested review from dham and connorjward May 21, 2025 16:00
Copy link
Contributor

@connorjward connorjward left a comment

Choose a reason for hiding this comment

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

In general looks really impressive. Nice job.

@KarsKnook KarsKnook requested a review from connorjward May 26, 2025 10:33
Copy link
Contributor

@connorjward connorjward left a comment

Choose a reason for hiding this comment

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

I am happy but this is a massive demo so I will bring to today's meeting to check with the others before merging.

@connorjward connorjward requested a review from pbrubeck June 4, 2025 15:36
:math:`v_i^\text{ref}` are reference velocities that we are free to choose.
Elsewhere on the boundary we enforce :math:`J_i \cdot N = 0`. Finally, instead of specifying
the value of the barycentric velocity :math:`v` on the inflows and outflows,
we enforce :math:`v = \rho^{-1}((J_1 + J_2)\cdot N)N`. Boundary conditions that couple
Copy link
Contributor

Choose a reason for hiding this comment

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

The inflow boundaries are normal to the y-axis. However the parabolic inflow has a component in the x-axis. This conflicts with the claim that you had before cross(v, N) = 0, and the current statement v = (scalar) N.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, thanks for catching that -- what we wrote here is correct for the 3D Benzene simulation from my paper but I guess we forgot that the BCs are slightly different in this 2D simulation.

I just pushed a commit that rephrases this paragraph, please resolve if you're happy with it.

ABaierReinio and others added 13 commits June 22, 2025 12:46
Clarify that (1 / specific volume) = density.

Co-authored-by: Pablo Brubeck <[email protected]>
Minor formatting changes

Co-authored-by: Pablo Brubeck <[email protected]>
Minor formatting changes

Co-authored-by: Pablo Brubeck <[email protected]>
Minor formatting changes

Co-authored-by: Pablo Brubeck <[email protected]>
Minor formatting changes

Co-authored-by: Pablo Brubeck <[email protected]>
Minor formatting changes

Co-authored-by: Pablo Brubeck <[email protected]>
Minor wording change

Co-authored-by: Pablo Brubeck <[email protected]>
Minor formatting changes

Co-authored-by: Pablo Brubeck <[email protected]>
Minor formatting changes

Co-authored-by: Pablo Brubeck <[email protected]>
Minor formatting changes

Co-authored-by: Pablo Brubeck <[email protected]>
Minor formatting changes

Co-authored-by: Pablo Brubeck <[email protected]>
pbrubeck
pbrubeck previously approved these changes Jun 23, 2025
Copy link
Contributor

@pbrubeck pbrubeck left a comment

Choose a reason for hiding this comment

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

LGTM

@ABaierReinio
Copy link
Contributor

@pbrubeck Thanks for all the suggestions. We are currently failing the tests because, when building the documentation, latex isn't aware of \mathscr. Can we get it to \usepackage{mathrsfs} (maybe by modifying https://github.com/firedrakeproject/firedrake/blob/master/docs/source/conf.py)? Alternatively we can use a different symbol font.

@pbrubeck
Copy link
Contributor

Feel free to make the change to support that latex command, or use another symbol

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants