Staggered grids in InvertPar #2087
Conversation
Previously was 'Create(Mesh*)' which was inconsistent with our case-convention and missing Options* and CELL_LOC arguments.
johnomotani
added
backport candidiate
|
I keep forgetting - do we have a list of breaking changes that I should add |
|
The fedora test is failing :-( I don't have a fedora system to test on. Could someone please check the latest |
|
I just merged a couple of PRs, I guess we'll see if the problem is also in CHANGELOG.md has a |
| return source | ||
| return re.sub( | ||
| r"({factory_name})\s*::\s*{old_create_method}\b".format(**factory), | ||
| r"\1::{create_method}".format(**factory), |
There was a problem hiding this comment.
Can we check that there's no argument to the Create method? The old version could take a Mesh* argument, which I'd be surprised if anyone was using, but if they were it'd be an error to pass as the first argument to the new create (which is an Options*). Don't think it's worth bothering to fix if there was an argument, but raising an error or a warning might be nice?
There was a problem hiding this comment.
Done. It's a bit dumb, in that it always warns if a factory where the arguments have changed is used. Hopefully the documentation is sufficiently useful for users to know what to do, otherwise we'll have to rely on the compiler. For me, using gcc 10.2, I get the following error (with nice colours too):
../tests/integrated/test-invpar/test_invpar.cxx: In function ‘int main(int, char**)’:
../tests/integrated/test-invpar/test_invpar.cxx:33:32: error: cannot convert ‘Mesh*’ to ‘Options*’
33 | auto inv = InvertPar::create(mesh);
| ^~~~
| |
| Mesh*
In file included from ../tests/integrated/test-invpar/test_invpar.cxx:8:
../include/invert_parderiv.hxx:104:53: note: initializing argument 1 of ‘static std::unique_ptr<InvertPar> InvertPar::create(Options*, CELL_LOC, Mesh*)’
104 | static std::unique_ptr<InvertPar> create(Options *opt_in = nullptr,
| ~~~~~~~~~^~~~~~~~~~~~~~~~
There was a problem hiding this comment.
looks good to me, thanks @ZedThree!
Default arguments mean it may only have a single argument
Allows
InvertParto work on staggered grids.CELL_YLOWversion requires changes to the boundary conditions. The Neumann boundary conditions have to be put in with one-sided differences, which (because the matrix inInvertParCRis tri-diagonal) are only first-order accurate. As long asInvertParis only being used for preconditioners, hopefully this should not be a problem.Also addresses a comment above the
InvertPar::Create()method that it should be calledcreateand takeOptions*(and nowCELL_LOC) arguments.Adds tests both for staggered grids and on open field lines (which were not previously tested).
InvertPar::create()supported inbin/bout-b5-factory-upgrader.py