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

Update torus output #127

Merged
merged 1 commit into from
Nov 27, 2023
Merged

Update torus output #127

merged 1 commit into from
Nov 27, 2023

Conversation

peterrum
Copy link
Member

@peterrum peterrum commented Nov 25, 2023

@kronbichler dealii/dealii#16287 seems to have introduced a bug. In test poisson/poisson_03.debug, we are using serial and p:f:T meshes; the output should not be influenced; but it is.

Is it possible that any line of https://github.com/dealii/dealii/blob/f1c6d6ed29af6f2fd62a613c40188e4b348ee31b/source/grid/grid_generator.cc#L2095-L2123 is now out of sync?

I will look into this issue. But maybe you have hunch what might caused the difference.

@kronbichler
Copy link
Contributor

dealii/dealii#16287 seems to have introduced a bug

Do we have reasons to believe it is a bug? Maybe you can state your reasoning along this direction? If I read the changes correctly, we here see some changes in the second/third digit of a residual after a multigrid solver has converged. All residuals become slightly smaller, so the trend is in the favor of the new code, so I would tend to believe that roundoff errors could also be the cause for this change. The question I have at this stage is whether we have some assessment of the quality of the solution itself, which can be expected to be less sensitive to roundoff than a residual. Some L2 error or similar could give us an indication of whether something got wrong in the code.

Regarding the code you are pointing to, can you state what you believe happens there and what parts it could affect? I think it is unlikely that this part can cause the present change, but I could very well miss something.

@kronbichler
Copy link
Contributor

As a follow-up statement, the new code should have all faces in standard orientation, whereas the old code did not. On the other hand, there will be some new face combinations (face number of a cell and the associated face number of a neighbor) that were not present before, so this would be something to check in our solver. But this test case seems to mostly use deal.II facilities, without many specific parts of the hyper.deal evaluators, right?

@kronbichler
Copy link
Contributor

Out of curiosity, does the new change dealii/dealii#16291 that also leads to a difference in the roundoff behavior (cell order changed, thus numbers accumulate differently) also lead to some changes in the numbers here? That would suggest that roundoff is an issue here. To phrase my expectations upfront, I would expect a lower change by the new PR, because only the order of additions of contributions from different cells is changed, whereas the first PR changed the roundoff behavior inside the cell/face integrals when degrees of freedom are passed through differently, which is potentially a factor 5-10 more severe in my experience.

@peterrum
Copy link
Member Author

@kronbichler Thank you for the comments.

I was looking into this topic yesterday, but did not finish the investigation yet. Let me post what I have till now. I am using peterrum@09b76e8 for the investigation for three version of deal.II (dealii/dealii#16252, dealii/dealii#16287, dealii/dealii#16291).

Running the test (simplified) gives the output:

// VERSION 1
DEAL:0::2.23030
DEAL:0::12.0845
DEAL:0:cg::Starting value 43.8178
DEAL:0:cg::Convergence step 7 value 0.000815558

// VERSION 2
DEAL:0::2.40544
DEAL:0::12.4538
DEAL:0:cg::Starting value 43.8178
DEAL:0:cg::Convergence step 7 value 0.000815146

// VERSION 2
DEAL:0::2.40544
DEAL:0::12.4538
DEAL:0:cg::Starting value 43.8178
DEAL:0:cg::Convergence step 7 value 0.000824243

The first two lines are the l2-norm of the diagonal vectors on the levels and the last two lines gives some statistics of the linear solver. One can see that each of them produce different end residuals. However, one can also observe that the diagonal vectors do not match.

To rule out as many possibilities for the differences (different eigenvalue estimates, ...), I have numbered the DoFs the same way (at least I hope I did) in the three cases. The output is now as follows:

// VERSION 1
DEAL:0::2.23030
DEAL:0::12.0845
DEAL:0:cg::Starting value 43.8178
DEAL:0:cg::Convergence step 7 value 0.000858381

// VERSION 2
DEAL:0::2.40544
DEAL:0::12.4538
DEAL:0:cg::Starting value 43.8178
DEAL:0:cg::Convergence step 7 value 0.000836674

// VERSION 3
DEAL:0::2.40544
DEAL:0::12.4538
DEAL:0:cg::Starting value 43.8178
DEAL:0:cg::Convergence step 7 value 0.000836674

The output of the newer versions of deal.II are now the same. However in the old version, the l2 norm of the diagonals are still off (and as a consequence the rest as well).

This is my status.

I would think that the diagonal should be the same in all three cases. But maybe that is not a correct assumption.

@kronbichler
Copy link
Contributor

We have this one:

VectorizedArrayType sigmaF =
(std::abs(
(phi.get_normal_vector(0) * phi.inverse_jacobian(0))[dim - 1]) +
std::abs((phi.get_normal_vector(0) *
phi_outer.inverse_jacobian(0))[dim - 1])) *
(Number)(std::max(fe_degree, 1) * (fe_degree + 1.0));

This computes the penalty factor from the first quadrature point, and since the orientation of faces has changed, we might get different values. This is more than roundoff as I expected (it also includes the estimate of the diagonal which is influenced by the DoF ordering by more than roundoff), so I could imagine that this explains it. One can use more expensive formulas for the penalty factor that are more robust, but in my opinion they do not pay off.

@kronbichler
Copy link
Contributor

In other words, given your details I think the difference between version 1 and 2 is the penalty factor and the changes in DoF numbering, and between version 2 and 3 the DoF numbering. Not quite what I thought, but it is due to how we build the test, not the change in the grid per se. I would update the output (including the diagonal norm as you did), add a comment and call it a day.

@peterrum
Copy link
Member Author

This computes the penalty factor from the first quadrature point, and since the orientation of faces has changed, we might get different values.

OK. The explanation makes sense! Thanks, @kronbichler!

It is good that we understand the root cause 👍

@peterrum peterrum merged commit a0e84b5 into hyperdeal:master Nov 27, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants