Add some basic tests for sheath_boundary_simple and refactor
#368
Conversation
`phi` must be calculated in field-aligned coordinates, and transformed to unaligned
Lower-y loop used wrong cell face
Possibly faster, certainly cleaner
ZedThree
added
boundary conditions
Didn't flip noflow BCs at same time, so was not exactly symmetric
ZedThree
force-pushed
the
sheath-boundary-testing
branch
from
45f3c45 to
083c0f0
Compare
| flipped_upstream = base_downstream | ||
| flipped_upstream["Ve"] = -flipped_upstream["Ve"] | ||
| flipped_upstream["Vi"] = -flipped_upstream["Vi"] | ||
| flipped_upstream["Si_sheath"] = -flipped_upstream["Si_sheath"] | ||
|
|
||
| expected = { | ||
| "upstream": flipped_upstream, | ||
| "downstream": base_upstream, | ||
| } |
There was a problem hiding this comment.
I solved the issue with switching which end had the sheath BC not being symmetric -- I needed to also switch the noflow BC to the other end. Now that I fixed that, it is exactly symmetric.
I just want to double check that the particle source Si_sheath should also flip sign -- is it a flux in the y-direction, rather than a scalar quantity? And should we expect that the flux is in the opposite sense to the velocities? (Particles moving into the boundary require a flux in the opposite direction to remove them and avoid a build up of density?)
There was a problem hiding this comment.
Unlike some other codes that impose the recycling flux on the boundary, we impose it as a source in the last domain cell. This is what Si_sheath is - a source (something per second per unit volume) and not a directional quantity like a flux. This means it needs to be positive at both ends.
Are you seeing a negative source at one of the ends?
There was a problem hiding this comment.
Yes, it looks like in this case, flipping which end has the sheath BC changes the sign of Si_sheath.
If Si_sheath should always be positive, should we add this as a check to the component?
I just wanted to let you know that that is essentially what I did for sheath_parallel: I think the plan is to only keep the parallel version, so too much refactoring is probably not worth it :-/ |
Yes, this way is very similar, although it's a complete free function, so doesn't rely on changes in BOUT++. I think a sensible plan is to get this in, and then look at refactoring it into BOUT++, and probably using regions like your version. We should very much try to avoid globals though.
I'm currently merging the different sheath boundary implementations into one, so I will also pull out your parallel version and merge it all together. This PR is laying the groundwork for that. |
* master: (54 commits) Silence some compiler warnings. Increase the test tolerance to work around compiler differences (seen with -O0 vs. -O2 and gcc@12+). fixed_velocity: Add description of mesh array to manual Typo in assert message. fixed_velocity: Read velocity from mesh Extend test input densities, temperatures outside clipped ranges. ADAS CX tests. Use common input ranges for all reaction regression tests. Rename test base class for ADAS izn/rec. Move AmjuelCXTest::generate_state() up to a parent class, so it can reused for ADAS. Rejig ReactionTest::sources_regression_test to check species data stored in (sub-)sections, including collision freqencies. evolve_pressure: Initialize flow_ylow if diagnose but no velocity Add H isotope CX tests. Move generate_data() for ADAS, Amjuel izn and rec tests into a separate class. Consistent naming of reaction regression data files. ADAS ionisation, recombination reaction regression tests. Correct a mistake in the docs Makefile that meant the doxygen target wasn't declared as .PHONY. Tidy up ReactionTest::generate_state(). Revert "Rename doxygen Makefile target to avoid clash with docs subdirectory name." Add missing header. ...
|
I've added a similar test for the insulating BC, along with a bug fix |
* master: (144 commits) Satisfy CMake when compiling with coverage flags Use the BOUT++ sanitizers cmake configs Collect coverage data only for unit tests Fix use of `LaplaceXY` for new BOUT++ version Trying to clarify how coverage reports displayed in PRs Fixed typo Trying to fix Codecov so it comments on PRs Bump bunded BOUT++ to current Add publication doi Update src/ionisation.cxx unit test evolve_momentum unit test evolve_pressure Switch to launch_safe FakeSolver: Mark run() override FakeSolver for unit testing of finally() methods snb_conduction: Test outputVars isothermal: Test outputVars ionisation: Merge radiation hydrogen rate Further debugging Fixed various config problems with code coverage ...
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #368 +/- ##
==========================================
+ Coverage 22.50% 26.09% +3.59%
==========================================
Files 87 88 +1
Lines 8071 8008 -63
Branches 1146 1129 -17
==========================================
+ Hits 1816 2090 +274
+ Misses 6085 5729 -356
- Partials 170 189 +19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ZedThree
force-pushed
the
sheath-boundary-testing
branch
from
2594a59 to
e066b24
Compare
mikekryjak
left a comment
mikekryjak
left a comment
There was a problem hiding this comment.
Looks good to me apart from the comment on flipping Si_sheath and the failed unit test.
| flipped_upstream = base_downstream | ||
| flipped_upstream["Ve"] = -flipped_upstream["Ve"] | ||
| flipped_upstream["Vi"] = -flipped_upstream["Vi"] | ||
| flipped_upstream["Si_sheath"] = -flipped_upstream["Si_sheath"] | ||
|
|
||
| expected = { | ||
| "upstream": flipped_upstream, | ||
| "downstream": base_upstream, | ||
| } |
There was a problem hiding this comment.
Unlike some other codes that impose the recycling flux on the boundary, we impose it as a source in the last domain cell. This is what Si_sheath is - a source (something per second per unit volume) and not a directional quantity like a flux. This means it needs to be positive at both ends.
Are you seeing a negative source at one of the ends?
density_boundary_mode = limit_freein the input fileyboundaries