- changed status to open
- removed comment
The ADMBase variables are not initialized after regridding
Ian Hawke: The point is that no BCs are applied to ADMBase variables, so they're never SYNC'd, so never initialized on refined grids.
Ian sent a patch which I extended at bit and attached to this ticket. Please review.
Keyword:
Comments (21)
-
reporter -
reporter - changed milestone to ET_2011_05
- removed comment
-
- removed comment
This does not apply an outer boundary condition (it is set to "none"). Maybe making the outer boundary condition a parameter would be better? I can imagine that either Dirichlet or von Neumann boundary conditions may be appropriate, depending on the situation.
-
- removed comment
A minor issue is that the boundary conditions are applied via a macro. Personally, I would prefer a static function for this.
-
reporter - removed comment
Can we accept the current patch for the next release and bump up the target of the ticket to the next release - to implement what you suggested?
-
- removed comment
Yes.
-
reporter - changed status to resolved
- removed comment
-
- changed status to open
- removed comment
-
- changed status to open
- removed comment
-
- changed milestone to ET_2011_11
- removed comment
-
- removed comment
I've added an additional patch that is relevant for the latest Mercurial version of Carpet (although the patch is again applied to ADMBase). In this version only the interior of the newly created grid is initialized by interpolation, so non-trivial boundary conditions need to be applied. This patch assumes that the original patch has been applied.
-
- removed comment
Please apply.
-
- assigned issue to
- removed comment
-
- removed comment
I am not clear about what need to be committed here. The first patch seems to have been committed already and the ticket was closed as "fixed" - was the patch applied at the same time? The ticket was then reopened and put in review state, and now there is a new patch here. Is it just the second patch which needs to be applied?
-
reporter - removed comment
The patch looks good, but I didn't try it.
-
reporter - removed comment
I would say - if it compiles and the testsuites pass, this is ok to commit.
-
- removed comment
This is a conceptual problem that has been around for many years. Test suites are not a good indicator of whether this is a good solution. As with all schedule changes, one has to be very careful.
-
- removed comment
OK I'm still a bit confused - should I apply the second patch, or do you now think it needs further review?
-
- removed comment
I think it's fine to apply, apologies for the confusion.
I believe only Ian Hawke's recent patch needs to be applied. Frank Löffler's much older patch has already been applied. I believe this is what is implied by comment
#5. -
- changed status to resolved
- removed comment
Committed in r62 of ADMBase.
I think it might not work with multipatch, because it uses GetBoundarySpecification instead of the Multipatch version (which takes the map as an additional argument). So if we ever need to use multipatch with these static evolutions we'll have to add the code to deal with that. I've tested that a multipatch simulation which uses non-static evolution still works.
-
reporter - removed comment
Replying to [comment:17 eschnett]:
Test suites are not a good indicator of whether this is a good solution.
That is right. However, they would at least test if the patch breaks any of the currently tested cases.
- Log in to comment