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

Implement an alternative regrid request strategy. #1147

Merged
merged 1 commit into from Nov 17, 2020

Conversation

drwells
Copy link
Member

@drwells drwells commented Oct 27, 2020

We can measure the displacement of the structure directly instead of relying on
estimates from the fluid solver.

As requested by @boyceg.

This is only implemented fully for IBFEMethod - using it in other classes will result in the program being aborted. If people know how to implement this for other IBStrategy objects they should write patches for them.

IBAMR Pull Request Checklist

  • Does the test suite pass?
  • Was clang-format run?
  • Were relevant issues cited? Please remember to add Fixes #12345 to close
    the issue automatically if we are fixing the problem.
  • Is this a change others will want to know about? If so, then has a
    changelog entry been added?
  • If new data structures have been added to a class then have they been set
    up appropriately for restarts? If so, ensure that the restart version
    number is incremented.
  • Does this change include a bug fix or new functionality? If so, a new test
    or tests should be added. New tests should run quickly (less than a minute
    in release mode). If possible, an older test should gain a new option so
    that we do not need to compile more test executables.
  • Did you (if your account has permission to do so) set relevant labels on
    GitHub for the pull request?

@drwells
Copy link
Member Author

drwells commented Nov 7, 2020

Rewriting this took awhile but its now done.

src/IB/IBFEMethod.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@boyceg boyceg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not call the Cartesian grid velocity-based estimate a "fluid velocity"-based estimate. (The velocity in a solid location is the solid velocity, not the fluid velocity! 😀 ) Suggest something like "Eulerian" or "background grid" velocity.

@drwells
Copy link
Member Author

drwells commented Nov 9, 2020

The first two commits in this PR are taken from #1158 - we can do everything at once but that PR is logically separate (though this one is dependent on the changes made there).

@drwells
Copy link
Member Author

drwells commented Nov 9, 2020

The velocity in a solid location is the solid velocity, not the fluid velocity!

Yeah, but we are using the fluid velocity, it's just inside the structure :)

@drwells drwells force-pushed the new-structure-displacement-strategy branch from e297435 to 44cdc38 Compare November 9, 2020 18:46
@drwells
Copy link
Member Author

drwells commented Nov 9, 2020

Either way - its now fixed.

@drwells
Copy link
Member Author

drwells commented Nov 17, 2020

@boyceg can you take a look soon? I don't want this to get stale.

include/ibamr/IBStrategy.h Outdated Show resolved Hide resolved
* Get the ratio of the maximum point displacement of all the structures
* owned by the current class to the cell width of the grid level on which
* the structure is assigned. This value is usually used to determine if the
* Eulerian patch hierarchy needs to be regridded.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Eulerian patch hierarchy needs to be regridded.
* Eulerian patch hierarchy needs to be regridded and/or if the assignment of Lagrangian data to Cartesian grid patches needs to be updated.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this isn't true - IBFEMethod deals with that problem (reassociating Lagrangian structures with the Eulerian grid) in an independent way. See IBFEMethod::preprocessIntegrateData().

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In particular: the displacement since the last regrid does not necessarily equal the displacement since the last patch-element association.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you help future me by adding to this comment by pointing to where this is handled and how this is distinct from the re-association trigger?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am stuck in the past, when regridding was the only way to update the associations between patches and elements.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was only three months ago :)

Copy link
Contributor

@boyceg boyceg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. A few minor suggested changes in wording in the comments. It might be more clear also to be able to allow users to explicitly specify a "fixed regrid interval" regridding strategy, and then the code use which ever triggers first?

@boyceg
Copy link
Contributor

boyceg commented Nov 17, 2020

Looks good. A few minor suggested changes in wording in the comments. It might be more clear also to be able to allow users to explicitly specify a "fixed regrid interval" regridding strategy, and then the code use which ever triggers first?

Specifically, I can imagine being confused if I were to set a fixed regrid interval and then don't get it because a CFL-like criterion also is set.

Since we now can specify multiple strategies for triggering regridding, would it make sense to add that one too?

@drwells
Copy link
Member Author

drwells commented Nov 17, 2020

Specifically, I can imagine being confused if I were to set a fixed regrid interval and then don't get it because a CFL-like criterion also is set.

The current behavior is to ignore the fixed regrid interval when the CFL-like condition is also set. I'll update the documentation to reflect the status quo. Do you want to change the way this works? That might break what people expect to happen.

@boyceg
Copy link
Contributor

boyceg commented Nov 17, 2020

Specifically, I can imagine being confused if I were to set a fixed regrid interval and then don't get it because a CFL-like criterion also is set.

The current behavior is to ignore the fixed regrid interval when the CFL-like condition is also set. I'll update the documentation to reflect the status quo. Do you want to change the way this works? That might break what people expect to happen.

I guess people may already be confused. 😀

@drwells drwells force-pushed the new-structure-displacement-strategy branch from 44cdc38 to bf56684 Compare November 17, 2020 18:49
@drwells drwells force-pushed the new-structure-displacement-strategy branch from bf56684 to df499fd Compare November 17, 2020 21:53
Copy link
Contributor

@boyceg boyceg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@drwells drwells merged commit 1457d2d into IBAMR:master Nov 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants