Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch reference tester to deal.II 9.5 #5498

Merged
merged 3 commits into from
Jan 27, 2024

Conversation

gassmoeller
Copy link
Member

We need to switch our reference tester to deal.II 9.5 to move forward with some other PRs (e.g. #5367, #5383). This is the first step. I still need to update the test results after the tester failed.

@gassmoeller gassmoeller force-pushed the update_reference_tester_to_9.5 branch from e5f8c3f to 978cd83 Compare November 16, 2023 20:15
@gassmoeller
Copy link
Member Author

I am chasing a bug in the zero_matrix test that I cannot seem to figure out right now. I am partly writing this as notes to myself (for when I can get back to this PR), but if anyone has ideas they would be appreciated.

What I noticed:

  • The zero_matrix.prm test does not trigger the correct assertion in solver.cc:401 anymore, and instead runs into a floating point exception. The assert ensures that system_matrix.block(block_idx, block_idx).linfty_norm() > std::numeric_limits<double>::min(), and should trigger for this test, because the matrix entries should all be zero. Clearly there are some matrix entries nonzero that should not be there (or where not there previously). The infinity norm of the matrix is 2.
  • I printed the matrix block and I see all entries are zero, except for entries on the main diagonal, which are either 0, 1, or 2.
  • I have figured out that I can remove the problem and correctly trigger the assertion that the matrix block is zero if I either (a) remove the temperature boundary condition, or (b) comment out line 699 in core.cc (where we call VectorTools::interpolate_boundary_values) for the temperature boundary condition.
  • Note that the boundary condition for T is set to 0 and the problem doesnt change even if I hard-code this (i.e. it is not a problem with the accuracy of the boundary condition function).
  • From the deal.II documentation on constraints (https://dealii.org/developer/doxygen/deal.II/group__constraints.html) I gather that the AffineConstraints class will always add main diagonal entries to the matrix for DoFs that are constrained.

So I guess my question is: If this is the case, why did this test work previously? Did deal.II change how constraints work internally? Or was our test broken in some sense?

I can update the test to work correctly if I just remove the boundary conditions for temperature, which will correctly lead to a matrix with only zero entries, which will then also trigger the Assert. Would this be the best way to handle this?

@tjhei
Copy link
Member

tjhei commented Nov 18, 2023

Interesting problem. I was not aware that something inside constraints has changed with 9.5. I do think it is correct/acceptable to have nonzero entries on the diagonal for constrained entries.

I will take a look soon, unless @bangerth has an idea.

@gassmoeller gassmoeller force-pushed the update_reference_tester_to_9.5 branch from 978cd83 to 60545f4 Compare November 27, 2023 16:49
@gassmoeller gassmoeller changed the title [WIP] Switch reference tester to deal.II 9.5 Switch reference tester to deal.II 9.5 Nov 27, 2023
@gassmoeller
Copy link
Member Author

For now I have disabled the prescribed bondary conditions in the failing test zero_matrix.prm and updated the test results. I think the test was broken before (at least the test output did not show the assertion that triggers if a matrix block is zero, but instead an assertion that triggers if an entry is not finite). So I think the change in deal.II must have happened before (I think the test was updated in ASPECT commit 7cf2fd1 in July 2020). So this test should now be fixed and better than before.

I also looked through all remaining tests, and while there are some changes they all look like typical changes that could come with a new deal.II version. I think this is ready to move forward.

The one remaining issue is the failing tester for deal.II master, but I think that is unrelated as well, and likely caused by dealii/dealii#16292. It should not be a problem for moving forward with this PR.

@gassmoeller
Copy link
Member Author

/rebuild

@bangerth
Copy link
Contributor

We've done a lot of work on AffineConstraints, but I thought that it was mostly after the 9.5 release. I don't know what specifically caused the issue here, but I think that it is correct now: If we have a constrained row, its diagonal entry should end up with something nonzero. Typically, we choose a value "roughly of the order of magnitude of other matrix entries", but since they are all zero here, we must be choosing 1.0 instead. Then, if you touch things from two adjacent cells, you add 1.0 twice, giving you the entries you observe.

@gassmoeller gassmoeller force-pushed the update_reference_tester_to_9.5 branch from 60545f4 to f5cdf82 Compare November 30, 2023 19:22
@gassmoeller gassmoeller force-pushed the update_reference_tester_to_9.5 branch from f5cdf82 to fde90f2 Compare January 26, 2024 19:05
@gassmoeller
Copy link
Member Author

I revisited this PR, rebased to the current main branch and reran the test locally. If the online testers succeed, I would like to move forward with this PR, because it blocks a number of other open PRs (#5367, #5383) that can be relevant for the user meeting (we should explain we are going to change the list of dependencies).

@bangerth
Copy link
Contributor

The dealii-master check of course fails, and the OSX tester is down. The rest seems to work, so I'll merge.

@bangerth bangerth merged commit 2e8b9d3 into geodynamics:main Jan 27, 2024
5 of 8 checks passed
@gassmoeller gassmoeller deleted the update_reference_tester_to_9.5 branch January 29, 2024 19:28
@gassmoeller gassmoeller restored the update_reference_tester_to_9.5 branch April 19, 2024 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants