The ADMBase variables are not initialized after regridding

Issue #333 closed
Frank Löffler created an issue

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.


Comments (22)

  1. Erik Schnetter
    • 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.

  2. Erik Schnetter
    • removed comment

    A minor issue is that the boundary conditions are applied via a macro. Personally, I would prefer a static function for this.

  3. Frank Löffler 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?

  4. anonymous
    • 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.

  5. Ian Hinder
    • 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?

  6. Frank Löffler reporter
    • removed comment

    I would say - if it compiles and the testsuites pass, this is ok to commit.

  7. Erik Schnetter
    • 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.

  8. Ian Hinder
    • removed comment

    OK I'm still a bit confused - should I apply the second patch, or do you now think it needs further review?

  9. Erik Schnetter
    • 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.

  10. Ian Hinder
    • 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.

  11. Frank Löffler 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.

  12. Log in to comment