-
Notifications
You must be signed in to change notification settings - Fork 78
Enable DUNE grid tests #903
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
base: master
Are you sure you want to change the base?
Conversation
|
jenkins build this serial please |
For this to work it is additionally needed that * Entities return the correct number of faces (to count intersections in the tests) * The refinement does not throw. Instead, it returns false when it cannot make a refinement. * DUNE is at least version 2.10 due to https://gitlab.dune-project.org/core/dune-grid/-/merge_requests/734
29fc137 to
36a7c00
Compare
|
jenkins build this serial please |
|
The CI does not seem to have DUNE 2.10, hence, this cannot be tested there. For now, I only can assure that this works locally, but it would be nice to have an automated test so that we can see if the test from DUNE are broken. |
| if ( cc == 2 ) { // Get number of lines of the element. | ||
| return 12; | ||
| } | ||
| if ( cc == 1 ) { // Get number of faces of the element. | ||
| return 6; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We clearly indicate in our capabilities that we do not support entities with these co-dimensions.
Shouldn't the check in DUNE honor that?
Even if we add this, then these numbers would not refelect any collapsed entities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To run the test yes. If I remember correctly, the test checks that the number of intersections is higher than the number of subentites of codimension 1. As I wrote in the footnote of the PR description, this just matches what we already say with the geometry type.
From my understanding, this function is just the short-hand of calling referenceElement<double,dimension>(type()).size(codim). IMO, the current behavior, which is returning 0, is a bug. This is independent on whether the types for entities of lower codimension can be instantiated or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed privately, you are right in the sense that dune should not require this information if the grid does not have entities of that codimension enabled. I am thus proposing a check that confirms that entities and reference element counts coincide, and a fix that makes dune use the reference element instead of the entity information.
Naturally, this would be wrong one we try more degenerate code. But I think this is a good first step.
I added you as a reviewer in the dune side: See https://gitlab.dune-project.org/core/dune-grid/-/merge_requests/794.
|
I convert this PR to be a draft because dune 2.11 requires the grid to fulfill the grid concept check. And this will not work until
|
For this to work it is additionally needed that
Footnotes
"Correct" here means with respect to the topology of a cube which is what the number of corners says. This is not correct for deformed entities though. Hence, this goes from wrong-wrong, to simply wrong. ↩