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

Use multiply_rho true for potential temperature evolution #1498

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

marchdf
Copy link
Contributor

@marchdf marchdf commented Feb 20, 2025

Summary

Use multiply_rho true for potential temperature evolution and adapt for multiphase sims.

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

Closes #1493

@marchdf
Copy link
Contributor Author

marchdf commented Feb 24, 2025

The following tests FAILED:
         25 - abl_stable (Failed)                               no_ci regression -> O(1e-10) diffs
         31 - abl_multiphase_laminar (Failed)                   no_ci regression -> machine diffs
         32 - abl_waves_terrain (Failed)                        no_ci regression -> machine diffs
         33 - act_abl_joukowskydisk (Failed)                    no_ci regression -> machine diffs
         34 - act_abl_uniformctdisk (Failed)                    no_ci regression -> machine diffs
         87 - hbl_godunov (Failed)                              no_ci regression -> O(1e-6) diffs
         95 - forest_drag (Failed)                              no_ci regression -> machine diffs
        107 - abl_meso_input_dpa (Failed)                       no_ci regression  -> machine diffs
        108 - abl_meso_input_ipa (Failed)                       no_ci regression  -> machine diffs
        109 - abl_meso_tendency (Failed)                        no_ci regression -> O(1e-6) diffs
        110 - abl_kosovic_neutral (Failed)                      no_ci regression  -> machine diffs
        112 - abl_kosovic_neutral_ib (Failed)                   no_ci regression -> machine diffs
        113 - nrel_precursor (Failed)                           no_ci regression -> machine diffs
        129 - abl_multiphase_w2a (Failed)                       no_ci regression -> O(1) diffs

what do we think of these @mbkuhn ?

@mbkuhn
Copy link
Contributor

mbkuhn commented Feb 24, 2025

fun. looks like I need to check the abl_stable, hbl_godunov, abl_meso_tendency, and abl_multiphase_w2a tests. I'm not too worried about the machine diffs. By the way, are these diffs between main and this PR, or are they diffs for this PR, before and after my commits to it?

@marchdf
Copy link
Contributor Author

marchdf commented Feb 24, 2025

these are diffs between main and this PR

@mbkuhn
Copy link
Contributor

mbkuhn commented Feb 25, 2025

For the reg tests with bigger diffs (listed below), I get the same diffs between main and your first commit e6eaab5, which just sets multiply_rho = true, as between main and the latest commit on this PR. These also match the diffs you report.

  • abl_stable
  • hbl_godunov
  • abl_meso_tendency

This means I was wrong about the implications of tweaking incflo_advance. That appears to have no effect, while multiply_rho is actually having a noticeable effect for certain cases. I'll still have to analyze, separately from this, where the large diffs in abl_multiphase_w2a are coming from.

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.

Discussion: set multiply_rho = true for temperature PDE
2 participants