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

in apply_bcs, only perform fillphysbc for new time #1303

Merged
merged 3 commits into from
Oct 21, 2024

Conversation

mbkuhn
Copy link
Contributor

@mbkuhn mbkuhn commented Oct 21, 2024

Summary

Avoid filling physbc at old times, they should already be filled. Necessary for #1302.

Pull request type

Please check the type of change introduced:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

Checklist

The following is included:

  • new unit-test(s)
  • new regression test(s)
  • documentation for new capability

This PR was tested by running:

  • the unit tests
    • on GPU
    • on CPU
  • the regression tests
    • on GPU
    • on CPU

Additional background

Should introduce no diffs, but may introduce diffs for abl_godunov_wenoz_fixedpt. However, this part of the algorithm needs to be adjusted for fixed point iterations, which I've talked to Ilker about.

@mbkuhn mbkuhn requested a review from marchdf October 21, 2024 17:55
@mbkuhn
Copy link
Contributor Author

mbkuhn commented Oct 21, 2024

The reason why this needs to be adjusted is the compute_diff_term always operates on the new state, even when the values are old. This isn't necessary; we should be able to operate on the old state, and that would fix things.

@marchdf
Copy link
Contributor

marchdf commented Oct 21, 2024

I am seeing these fail:

The following tests FAILED:
          7 - abl_godunov_mpl (Failed)
          8 - abl_godunov_mpl_amr (Failed)
         60 - abl_godunov_wenoz_fixedpt (Failed)
        121 - abl_bndry_input_native (Failed)
        123 - abl_bndry_input_amr_native (Failed)
        124 - abl_bndry_input_amr_native_xhi (Failed)
        125 - abl_bndry_input_amr_native_mlbc (Failed)
        126 - abl_godunov_forcetimetable (Failed)
        127 - abl_bndry_input (Failed)
        128 - abl_bndry_input_amr (Failed)
        129 - abl_bndry_input_amr_inflow (Failed)
        130 - nrel_terrain (Failed)

The fixedpt one is just missing golds. The others are diffing.

@mbkuhn
Copy link
Contributor Author

mbkuhn commented Oct 21, 2024

I think these all make sense, actually. The boundary planes are all currently hard-coded to use new_time when there is a fillphysbc. Therefore, when apply_bcs takes place at the old time, it puts new data in to the boundaries of the new state, which hasn't been updated yet, so it is just old data with new boundaries. This PR takes that away.

Ok, well, one thing that pokes a hole in this is that many of these cases have incflo.diffusion = 2, which means it's pure implicit. Not sure what compute_diff_term is doing in that case, gonna have to investigate further.

@mbkuhn
Copy link
Contributor Author

mbkuhn commented Oct 21, 2024

Ok, I stand by my original theory. Even though many of these use implicit diffusion, the mac projection still uses the explicit diffusion calculation. The diffs only show up where the boundaries are coded to update (even when they shouldn't, at old times).

@mbkuhn
Copy link
Contributor Author

mbkuhn commented Oct 21, 2024

Supporting evidence:
if I change the state to "New" instead of "Old" in this line, then the diffs go away. The changes only prep diffusion terms for the MAC extrapolation step.
https://github.com/Exawind/amr-wind/blob/main/amr-wind/incflo_advance.cpp#L250

@mbkuhn mbkuhn merged commit 8972c0f into Exawind:main Oct 21, 2024
15 checks passed
@mbkuhn mbkuhn deleted the avoid_old_fill branch October 22, 2024 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants