Add DAGP_fv for FCI - #3478
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
ZedThree
left a comment
ZedThree
left a comment
There was a problem hiding this comment.
Looks good, a few comments though. Also:
- please expand the docs
- needs tests -- should be pretty easy to add to
fv_mms? - please can we change the name? :)
|
|
||
| /// Div ( a g Grad_perp(f) ) | ||
| /// | ||
| /// This version uses pre-computed coefficient. It can also be used |
There was a problem hiding this comment.
I think there's some text missing?
It could definitely do with some more docs, I guess that g is precomputed here given that the call operator only takes a and f?
Certainly we need something about how to create one
| /// Div ( a g Grad_perp(f) ) | ||
| /// | ||
| /// This version uses pre-computed coefficient. It can also be used | ||
| class dagp_fv { |
There was a problem hiding this comment.
This is in the FV namespace, right? So we can drop the _fv (unless it doesn't stand for finite volume here?)
I'm also not a big fan of the name -- all our other operators are spelt out (well, most of them. I guess Delp2 is not great!) which helps the readability of the physics models.
So can we go for:
| class dagp_fv { | |
| class Div_a_g_Grad_perp { |
|
|
||
| private: | ||
| template <bool extra, bool upwinding> | ||
| Field3D operator()(const Field3D& a, const Field3D& f, Field3D* low_xlow, |
| BoutReal xflux(const Field3D& a, const Field3D& f, const Ind3D& i); | ||
| template <bool upwinding> | ||
| BoutReal zflux(const Field3D& a, const Field3D& f, const Ind3D& i); |
| result /= volume; | ||
| return result; |
There was a problem hiding this comment.
| result /= volume; | |
| return result; | |
| return result / volume; |
| Field3D operator()(const Field3D& a, const Field3D& f, Field3D& low_xlow, | ||
| Field3D& flow_zlow, bool upwinding); | ||
| Field3D operator()(const Field3D& a, const Field3D& f, bool upwinding); |
There was a problem hiding this comment.
Should these be const?
low_xlow and flow_zlow need some docs -- not sure that flow is a great name either, I think it's f_low rather than flow?
| volume /= fac * fac; | ||
| return *this; | ||
| } | ||
| dagp_fv& dagp_fv::operator/=(BoutReal fac) { return operator*=(1 / fac); } |
There was a problem hiding this comment.
Is this used anywhere? Or is it anticipated to be used in user code?
| std::map<Mesh*, std::weak_ptr<dagp_fv>> dagp_fv_cache; | ||
| } | ||
|
|
||
| std::shared_ptr<dagp_fv> getDagp_fv(Mesh* mesh, BoutReal rho_s0) { |
There was a problem hiding this comment.
If this were instead a static method on the class (and named something like create?), could we inline operator*= here and drop it? Or is it also needed elsewhere?
2 participants
Add the DAGP_fv operator needed for boutproject/hermes-3#643