Skip to content

Port some Hermes-3 finite volume operators (rebased)#3335

Open
dschwoerer wants to merge 45 commits into
nextfrom
move-ops
Open

Port some Hermes-3 finite volume operators (rebased)#3335
dschwoerer wants to merge 45 commits into
nextfrom
move-ops

Conversation

@dschwoerer

Copy link
Copy Markdown
Contributor

Rebase of #3200 on top of #3320 and #3334

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

Comment thread include/bout/deriv_store.hxx
Comment thread include/bout/deriv_store.hxx
Comment thread include/bout/deriv_store.hxx
Comment thread include/bout/field3d.hxx Outdated
Comment thread include/bout/field3d.hxx
Comment thread src/mesh/difops.cxx
Comment thread src/mesh/parallel/fci.cxx Outdated
Comment thread src/mesh/parallel/fci.cxx
Comment thread src/mesh/parallel/fci.cxx Outdated
Comment thread src/sys/derivs.cxx
@ZedThree

Copy link
Copy Markdown
Member

A couple of points that haven't been done:

  • @mrhardman pointed out some other operators that could potentially be ported, along with their checks Port some Hermes-3 finite volume operators #3200 (comment)
  • there's now a lot of code in the fv_ops.hxx header -- if the set of possible CellEdges implementations is closed to what's in BOUT++, it would be possible to shift stuff out of the header and provide explicit instantiations of the template functions. This would probably speed up compilation, at the cost of being harder to add new limiters

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

Comment thread src/mesh/difops.cxx Outdated
Comment thread src/mesh/difops.cxx
Comment thread tests/MMS/spatial/finite-volume/fv_mms.cxx
@dschwoerer

Copy link
Copy Markdown
Contributor Author

A couple of points that haven't been done:

@mrhardman pointed out some other operators that could potentially be ported, along with their checks Port some Hermes-3 finite volume operators #3200 (comment)

Sorry, I tried to follow to the comment, and from there to two PRs, but I failed to see what operators you would like to add here?
Do you also want to add them to these tests, or do you want to also port some of the non-orthogonal mesh tests to BOUT++?
I am not objecting to that, but could this be a separate PR?
This one already "blocks" #3392 and #3405.

there's now a lot of code in the fv_ops.hxx header -- if the set of possible CellEdges implementations is closed to what's in BOUT++, it would be possible to shift stuff out of the header and provide explicit instantiations of the template functions. This would probably speed up compilation, at the cost of being harder to add new limiters

✔️ done in #3405

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

Comment thread include/bout/fv_ops.hxx
#include "bout/assert.hxx"
#include "bout/bout_types.hxx"
#include "bout/boutexception.hxx"
#include "bout/build_defines.hxx"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: included header boutexception.hxx is not used directly [misc-include-cleaner]

Suggested change
#include "bout/build_defines.hxx"
#include "bout/build_defines.hxx"

Comment thread include/bout/fv_ops.hxx
#include <bout/mesh.hxx>

#include <cmath>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: included header cmath is not used directly [misc-include-cleaner]

Suggested change

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.

3 participants