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
Functor material properties #18395
Functor material properties #18395
Conversation
Job Documentation on 0653f11 wanted to post the following: View the site here This comment will be updated on new commits. |
22e5abd
to
ccf08d1
Compare
This has some really good ideas. And a few things that I'm uncomfortable with. I love that we get some of that on-demand property calcs stuff I've been wanting. But I am concerned at the limitations of your |
You can add new geometric overloads at any time. For instance today I will add a |
0b85090
to
6822778
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GiudGiud I'm curious what you think about this commit message
looking at the commit rather than the message, wrt to INSFV you have not changed the behavior? not sure how we would evaluate rho * v directly (eg not rho_f v_f) on the face unless we make it the variable instead of v |
The behavior should be changed. In the interior we would interpolate rhou together before, e.g. rhou_avg = (rho_elem * u_elem + rho_neighbor * u_neighbor) / 2. Now rho and u are interpolated separately and multiplied, e.g. rhou_avg = (rho_elem + rho_neighbor) / 2. * (u_elem + u_neighbor) / 2.
Correct. Right now with the functor implementation this would be the only way to "interpolate them together" |
Oh rho_u used to be the advected quantity, true true |
3b66934
to
42771c5
Compare
btw I was thinking you were going to evaluate rho(T_f, P_f) at the face, not really (rho_elem + rho_neighbor) / 2. |
Yes any call to evaluate rho at a face will result in a query of fluid properties for rho_from_T_p(T_f, p_f). However a user could still ask for rho_elem and then they would get rho_from_T_p(T_elem, p_elem) |
@joe61vette thinks are looking promising. Here are for a couple different Rayleigh numbers |
I would say it looks more than a little promising. I also prefer keeping to the FVMatAdvection scheme whenever possible. It is so easy (using the AD system) to build up compound material properties (eg, alfarhou) that can be combinations of nonlinear variables and fluid properties to be advected. |
It is still easy to build compound properties with the functor property scheme and it halves the necessary data members in residual objects. But if being able to do interpolations together, in the sense that (rhou)_avg != rho_avg * u_avg, is important, then that is a technical issue that would require more thought. |
I didn't make my comment very clear (sorry). For the interpolations, I would want to interpolate between adv_quantity_elem and adv_quantity_neighbor. Where adv_quantity_elem would be a compound material property (eg, alfa * rho * u / eps). Where alfa & u would be non-linear variables (volume fraction and superficial velocity), rho would be a fluid property (fn(P,T)), and eps would be an AuxVar for the porosity. I believe this is what you are proposing. |
So, functionally speaking, you can do this very easily with the current functor implementation, but your result would be this: interp(alfa * rho * u / eps) = interp(alfa) * interp(rho) * interp(u) / interp(eps) If this is not desirable to you, then I can think harder about how to do aggregated interpolations with the functor system |
It has been a long time since I have looked into the proper way to go from the PDEs to the finite volume formulation. But I believe that interpolating each quantity separately could lead to problems. For example, let's consider a compound material property say "momentum_x", for our conditions momentum_x_elem < momentum_x_neighbor. If we interpolate each quantity separately, I don't believe there is a guarantee that: momentum_x_elem < momentum_x_face < momentum_x_neighbor I admit that I am not sure about this. Perhaps @GiudGiud can weigh in as well. At any rate, I thought this was the direction you were headed based on the above comment: "The behavior should be changed. In the interior we would interpolate rhou together before, e.g. rhou_avg = (rho_elem * u_elem + rho_neighbor * u_neighbor) / 2. Now rho and u are interpolated separately and multiplied, e.g. rhou_avg = (rho_elem + rho_neighbor) / 2. * (u_elem + u_neighbor) / 2." |
|
How is that? In my head there's the same number of operations |
My hatred of performing material property evaluation "at neighbor" stems from the fact that it requires material property ghosting which has raised a whole slate of issues for us both in terms of code maintenance and in terms of user input file construction. If we can train developers to think about what they do at domain boundaries when using FVFluxKernels then some-day we could remove the need to do material property ghosting |
I just don't like ghosting period. If we are at a domain boundary, we should evaluate a quantity based on Dirichlet information or with |
f3287fa
to
c7f3cab
Compare
Also clean-up documentation to reflect that fully compressible only works with a momentum parameter and incompressible only works with a density parameter and superficial velocity variable set
Ok I think I hit all the new requested tests. @GiudGiud I edited #18395 (comment) to reflect what I added tests for and where I removed code. I may add another test for when a property is initially retrieved as an ordinary functor property and is later declared as another property type |
Removing standalone NSFV and PNSFV for gravity and friction
db87895
to
e5c5bfe
Compare
cant resolve all the conversations from the files tab btw @loganharbour this will need to break CI with Pronghorn |
Hold on, in the process of increasing coverage, I found a bug 😆 Boy, who would ever complain about a code coverage target! They must be really dumb! |
This revealed what should not have been a surprise: deleting an object makes any reference access to that object bad access. So now I use the letter and envelope idiom to allow changing out of the underlying functor material property type
It seems like @GiudGiud is at the point of acceptance on this. @rwcarlsen @friedmud ? |
🍻 |
At this point - any input of mine would not be useful. We can continue to evolve this system as we go. Good work all! I approved the changes... |
Ok I'm going to give another day on this, and then I'm going to dismiss @rwcarlsen 's review unless he indicates otherwise |
@rwcarlsen currently unresponsive
@GiudGiud @rwcarlsen @joe61vette
As per bec76f3 I think I should make a
FunctorMaterial
or something that doesn't get added in triplicate like our currentMaterials
doTo-do:
FunctorMaterial
version of, for instance,GenericFunctionMaterial
Closes #16809