search

boutproject / BOUT-dev

BOUT++: Plasma fluid finite-difference simulation code in curvilinear coordinate systems
http://boutproject.github.io/
GNU Lesser General Public License v3.0
182 stars 95 forks source link

Staggered grids meta-issue #820

Open ZedThree opened 6 years ago

ZedThree commented 6 years ago

There are currently a few problems with using staggered grids (e.g. #64, #259, #411, #741, #810). It's not clear (to me, at least) if these are related issues and staggered grids are completely broken, or if they just need a bit of polish and tightening up.

We're also missing decent tests for them. There is examples/staggered_grid, but it doesn't really tell us if staggered grids are working correctly or not. First task, then, should be to get some test coverage that highlights any current problems and ensures that they don't creep back in.

Once we know what's a bug and what's a design issue, we can work on fixing everything.

dschwoerer commented 6 years ago

742 comes with some tests, and avoids some of the issues, e.g. #810 #741 and #259

It further has tests of the derivative, to make sure they converge with the expected accuracy.

ZedThree commented 6 years ago

:+1: Do you want to see if you can pull out as many of the tests as you can into a separate PR?

dschwoerer commented 6 years ago

No.

Long version: Is there no interest to merge #742 ? That takes time to separate. Pulling out causes further merge conflicts, wasting even more time I would prefer to spend my time otherwise. Unless #742 is rejected, I cannot see an advantage.

ZedThree commented 6 years ago

Yes.

Long version: no, not in its current state:

1) It currently modifies 16k lines across 67 files and 109 commits -- that's a lot to review and understand! It's no good merging something if only one person understands the code and can fix problems with it. What happens when that person moves on to other projects? See technical debt/bus factor.

2) It introduces several unrelated features/bug-fixes -- these would be useful as standalone PRs. PRs that introduce discrete (or a set of related) features/bug fixes are easier to review, easier to understand, and are more likely to get merged faster.

3) We have other priorities -- none of the maintainers actually gets to spend all of their time on BOUT++, and some of us are contractually obliged to work on particular things. While we do want to merge the Aiolos mesh (and the Cython stuff too), we, like you, want to spend our limited time on the projects we ourselves are working on. Bigger PRs mean we have to spend more time understanding them, and get less time to work on our own projects.

Honestly, we do want to merge #742 and #641, but they are big PRs. Pulling out unrelated features means that those features get in faster, and we'll end up with a smaller, easier to understand PR.