Fix Ubuntu-System-CI #357
Fix Ubuntu-System-CI #357
Conversation
Change number of diffusion iterations, boundary sizes and resolution of EulerDirichletBoundaries. Changed number of diffusion iterations for EulerNeumannZeroBoundaries. Changed number of diffusion iterations for EulerNeumannNonZeroBoundaries.
|
Hey @ryan-bournes , I saw you assigned me and @nicogno as reviewers. The PR is supposed to fix the Ubuntu System CI, however, it still fails. Please take a look at the log. You'll probably have to add to |
Adding suppression to fix memory leak during ubuntu 20.04 unit tests.
|
Ping us when you're ready here @ryan-bournes . |
|
Hi @TobiasDuswald , the changes seem to have passed the ubuntu checks but has failed the macOS and Centos checks, will this be an issue? |
There was a problem hiding this comment.
Thanks @ryan-bournes . Can you double check that the one unit test still does what it is supposed to do (see review comment). Issues on CentOS / macOS can be ignored for now (except if they result from the unit test changes of course).
| @@ -1045,7 +1045,7 @@ TEST(DiffusionTest, EulerNeumannZeroBoundaries) { | |||
| rm->AddContinuum(dgrid); | |||
|
|
|||
| // Simulate diffusion / exponential decay for `tot` timesteps | |||
| int tot = 10000; | |||
| int tot = 1000; | |||
There was a problem hiding this comment.
Have you checked if the concentration still reaches the boundary @ryan-bournes ? This test is supposed to verify that nothing escapes with homogeneous Neumann conditions, the test is only meaningful if the concentration reaches the boundaries. Possibly a good idea to lower int tot but add a source close to each of the six boundaries.
|
@nicogno if you found time to check this PR that would be great, it affects the unit tests you designed in the scope of the boundary conditions :) |
Reduced simulation size and moved sources to edges of simulation space.
|
@TobiasDuswald I expect this test to pass regardless but I have modified it so the sources are right next to the boundaries. |
|
|
Hi @ryan-bournes, I agree with @TobiasDuswald on the comments about the changes! Everything looks good to me, as long as you double check that the concentration reaches the boundaries in the Neumann Zero Boundaries test |
It looks like they're affected by some missing dependency. |
yes, none of the concerns for this PR. See also development branch related to #356 . |
Change number of diffusion iterations, boundary sizes and resolution of EulerDirichletBoundaries.
Changed number of diffusion iterations for EulerNeumannZeroBoundaries.
Changed number of diffusion iterations for EulerNeumannNonZeroBoundaries.