Remove old backward compatible code with Boundary #5932
Conversation
|
/run-tests |
|
9.0 is supposed to still support using This should fix #5931 but I am not sure if it is possible to fix that bug and also preserve compatibility with the old way of doing curved boundaries. |
|
we support Boundary objects. We no longer support curved parts of the domain defined by boundary_ids. |
|
that is, if you attach a boundary object, you can still do it, but you are now obliged to use |
|
Thanks @luca-heltai, that seems to have done the job. Do you mind if I push a unit test to your branch? |
|
not at all. Go ahead! |
|
Whoops, there's a bit more fallout than I would have expected (or hoped for)... I guess @drwells was right about this breaking compatibility. |
|
I took a look at |
Well, this is behaviour that has to change. |
|
I agree with @luca-heltai. The |
|
Thanks for working on the broken tests and documentation @luca-heltai. I'd like to have a look through the changes, but only tomorrow if you don't mind. |
|
I'll let the tester talk. There will be other broken tests, I fear. i didn't check them all. Hopefully less than what they were... |
|
Well, if there are more tests that need some TLC then I'll try to contribute some fixes as well. |
|
Arg, so much for getting to this yesterday. Sorry :-/ I will endeavour to fix some of the tests on the weekend... |
|
@luca-heltai I think that we'll have to adjust the remaining tests by hand. I guess that many of them rely on the boundary and manifold ID's automatically having the same value, regardless of the colorize flag being used. e.g. in if (dim==2)
{
GridGenerator::hyper_ball(tr, Point<dim>(), 1);
}
else
GridGenerator::hyper_cube(tr, -1./std::sqrt(static_cast<double>(dim)),1./std::sqrt(static_cast<double>(dim)));
// GridTools::copy_boundary_to_manifold_id(tr); // Should slot this in here
static const SphericalManifold<dim> boundary;
if (dim != 1)
{
tr.set_manifold (0, boundary);
}I'll try to work through these during the course of the week. |
|
@jppelteret I'm not sure what to do... the main source of troubles stems from the fact that the default boundary indicator is 0 for all grids, while the default manifold indicator is Should we make the decision that the default manifold indicator on the boundary should be 0? If this is so, there is one single place to change, which is when we create the triangulation and the initial manifold indicator on the boundary faces. I would be in favour of having the user specify explicitly a manifold indicator, if he so wishes, either by setting colorize to true, or by doing it by hand. |
|
The second choice is naturally more expensive... we'd have to change manually 100 tests... |
I think that this would be a safe thing to do since |
|
On 03/05/2018 03:07 AM, Jean-Paul Pelteret wrote:
Should we make the decision that the default manifold indicator on the
boundary should be 0? If this is so, there is one single place to change,
which is when we create the triangulation and the initial manifold
indicator on the boundary faces.
I /think/ that this would be a safe thing to do since |0| is a reserved
indicator id (to the best of my recollection, you can't call
|set_boundary_id(0)|).
I'm pretty sure that you can -- in fact, I wouldn't see a reason why
disallowing this would make sense.
If I recall correctly, then the default boundary_id is indeed zero. But the
whole goal of what we've been doing is to disentangle boundary_ids and
manifold_ids. Also to make dealing with manifolds simple and uniform. So why
make the default manifold_ids for geometric objects that happen to be located
on a boundary different than objects that are not? This seems "not simple and
uniform".
|
|
I agree with you @bangerth , that's why I have not yet really put myself into this PR... What is your suggestion? Would you agree that colorize should assign both manifold and boundary ids? This seems reasonable (and this PR does only this, so far), so users can attach manifolds to the boundary, and/or use boundary conditions judiciously. The main problem lies in the fact that most of our tests exploit the old convention. I'm willing to add all necessary calls to |
|
What is your suggestion? Would you agree that colorize should assign both
manifold and boundary ids? This seems reasonable (and this PR does only this,
so far), so users can attach manifolds to the boundary, and/or use boundary
conditions judiciously.
I think we ought to figure out what colorization is supposed to solve. I don't
actually really know,. For some of the GridGenerator classes, it clearly made
sense to assign different boundary indicators that correspond to different
boundary objects (e.g., for things such as half_hyper_shell()). In those
cases, it would make sense to also set the manifold_id in a similar way.
But then that doesn't seem to apply to cases such as hyper_cube(). Maybe the
concept of colorization wasn't fully thought through?
In any case, there is likely little harm in setting *both* boundary and
manifold id. That's because if you set a manifold_id but don't *explicitly*
attach a manifold that corresponds to this manifold_id, you just get the
default flat manifold. So no harm is done.
The main problem lies in the fact that most of our tests exploit the old
convention. I'm willing to add all necessary calls to
|copy_boundary_to_manifold_ids| in the tests, if this is the right way to go...
That would be another option, though it doesn't seem wrong to me to set both ids.
|
|
Independent of the question in the user group this morning, I started thinking about this again. My first thought was that we could do is adjust to boundary to manifold id copy function in template <int dim, int spacedim>
void copy_boundary_to_manifold_id(Triangulation<dim, spacedim> &tria,
const bool reset_boundary_ids=false,
const bool retain_flat_manifold_ids=true);but I don't think that this would work for template <int dim, int spacedim>
void copy_boundary_to_manifold_id(Triangulation<dim, spacedim> &tria,
const std::vector<types::boundary_id> retain_flat_manifold_id_on_boundary,
const bool reset_boundary_ids=false);would be more flexible. I'd be happy to implement and integrate this if its agreed to be the right approach. |
|
Correction: I meant To be more general, this parameter could be called |
|
I suspect that I will be outvoted on this one (which is fine) but I would like to at least state my opinion regarding how we should proceed. The proposed compatibility break is very large. I suspect that users are already working around this bug in their own grid generator functions and changing the colorization and boundary handling will change their code. I know that we deprecated this behavior a long time ago, but given the large number of failing tests (and the fact that we need to redo boundary/manifold ID handling for all |
|
@drwells , I think this has waited long enough. I think we ought to have a solid distinction between Manifolds and boundary conditions in 9.0. I personally think that we should not fear breaking compatibility in our test suite. It is good that 100 tests fail, because it means we are stressing this part of the library at least with 100 different tests. I'd vote the following
|
|
I agree with all of this. In particular, I'm in favor of completely separating boundary indicators and manifold ids. The fact that we ever treated them the same is a historical accident, and at one point or other, we will have to bite the bullet and make them separate. The question is then just how we deal with the large number of failing tests. |
There was a problem hiding this comment.
I've looked this through and I like it. If we can find a way to fix up the various tests, that would be fantastic and then this can be merged.
@luca-heltai -- I particularly appreciate you taking the time to properly document what each of the grid generator functions does with the boundary and manifold ids.
|
I'm waiting for #6182 to be merged, before I proceed with this. |
|
It looks like the consensus is to go forward with this change. We need to fix 94 failing tests: I am willing to help with thi process. How about I fix the tests in |
|
before we proceed, let me make an alternative PR, where I don't simply copy over, but make sure that manifold ids are set if and only if they are necessary. |
Yes, that makes sense. Things to keep in mind:
I think it is annoying having to manually assign different boundary ids after the fact but it is also annoying having to specify boundary conditions for a colorized Another thought might be what we do in ASPECT: we give string names to the individual boundaries (like "left", "cylinder", etc.). Internally, they could be mapped back to ids... |
for now, this is just boundary. We could (in the future), allow one to set input flags to import single ids as manifold instead. For the moment no attempt at traducing this to manifold information was done.
The same way it was done before. Simply now it has to be interpreted as something related to boundary conditions only, and has nothing to do with geometry.
This was my main reason to rethink this PR and go towards #6233. Colorize should be used for boundary ids. There is no need to colorise manifold ids. We know what geometry we are generating, so the manifold is always set to the correct value, and the right Manifold is attached, for each triangulation we construct.
Maybe not in this PR... what we want to do for boundary condition indicators should have nothing to do with manifold indicators, and PR #6233 does not impact any decision you may want to take later on for boundary ids.
Again, maybe not in this PR. |
|
I am trying to get up to date with these boundary patches. Does #6233 supercede this PR? |
Fixes #5931